C++

Forum

 
Set Implementation Project Sanity Check

dakotad8218 (50)
Hi, I know asking for help with homework is usually frowned upon here, but considering the fact that it's already complete and turned in, I just wanted to see if anyone a little more proficient than myself had any suggestions/critiques of the way I wrote this. It's basically just teaching you how to overload operators to handle different class objects. Ours were just class Set and it basically emulated an unordered set. The requirements were to create the class, add member functions to add ints to the Set, remove ints from the set, and overload the operators & and | to create a union or intersection between the two sets.

Thanks in advance for any insights. It's one of those things where I wrote the code, it works, but it felt like it was too easy and I was cutting corners somewhere but can't quite identify where. I'm a CIS major so I want to actually learn the "right" way to do things, not just write code that "works." Probably a good idea as I am planning on coding for a living.

Here's my "set.h"
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
 
#ifndef SET_H
#define SET_H
#include <iostream>
#include <vector>
#include <string>

class Set 
{
	public:
		Set();
		Set(std::string);
		void add(int);
		void remove(int);
                void print();
                std::vector<int> get_set();
                std::string get_name();
                bool num_present(int, std::vector<int>);
	private:
		std::vector<int> num_set;
		std::string name;
};

#endif


set.cpp:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
 
#include "set.h"
#include <iostream>
#include <string>
#include <vector>

Set::Set()
{
	//default (empty) constructor
}

/*
 * Default constructor with name parameter
 * @param name - name of number set
*/
Set::Set(std::string name)
{
	this->name = name;
	//uncomment next line to indicate object creation in ostream
  //std::cout << "Created number set " << name << "!" << std::endl;
}

/*
 * Returns the private data member std::string name;
 * @param void
 * @return name : name of number set (set by default)
 * constructor with string parameter
*/
std::string Set::get_name()
{
    return name;
}

bool Set::num_present(int newNum, std::vector<int> set)
{
    bool found = false;
    for(int i = 0; i < set.size(); i++)
    {
      if(set[i] == newNum)
      {
        found = true;
      }
      if(found)
       {
         break;
       }
    }
    return found;
}

/*
 * Function to add number to set
 * @param newNum - new number to add to the set vector
*/
void Set::add(int newNum)
{
  if(!num_present(newNum, num_set))
  {
	  num_set.push_back(newNum);
  } 
}

/*
 * Removes a number from Set object
 * @param rNum - integer to remove from set vector
*/
void Set::remove(int rNum)
{
  for(int i = 0; i < num_set.size(); i++)
  {
    if(num_set[i] == rNum)
    {
      num_set.erase(num_set.begin() + i);
    }
  }
}

/* Retrieves a the number set as a vector for the Set object
 * @param void
 * @return o_set - Usable vector
*/

std::vector<int> Set::get_set()
{
	return this->num_set;
}

/*
 * Prints the selected number set object to the console.
 * the "Old Fashioned Way" of printing out the set without
 * operator overloading. Can be slightly modified
 * to actually serve as overload function.  Kept here
 * as a redundancy just in case.
 * @param void
 * @return void
*/
void Set::print()
{
  std::cout << "\"" << name << "\" is as follows: {";
  for(int i = 0; i < num_set.size(); i++)
	{
    //avoid placing a comma after the last number in the set
    if(i == num_set.size() - 1)
    {
      std::cout << num_set[i];
    } else {
      std::cout << num_set[i] << ", ";
    }
	}
	std::cout << "}" << std::endl;
}


and finally my main.cpp
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
 
#include <iostream>
#include "set.h"

/*
 * Overload the output stream operator to accept
 * Set objects
 * @param out : ostream reference
 * @param set : Set object reference
 * @return out: ostream object passed by reference
*/
std::ostream& operator<<(std::ostream& out, Set set)
{
  std::vector<int> buff = set.get_set();
  out << "{";
   for(int i = 0; i < buff.size(); i++)
	{
    //avoid placing a comma after the last number in the set
    if(i == buff.size() - 1)
    {
      out << buff[i];
    } else {
      out << buff[i] << ", ";
    }
	}
	out << "}" << std::endl;

  return out;
}

/*
 * Overload the | operator to produce
 * a union of two sets and return
 * a new, combined Set object
 * @param result : newly generated unified set
 * @param first : first Set object
 * @param second: second Set object
 * @return result : third, unified set object
*/
Set operator|(Set& first, Set& second)
{
  Set result;
  std::vector<int> x = first.get_set();
  std::vector<int> y = second.get_set();
  
  /*
   * Merge set vectors into one that can be more easily 
   * worked with.
  */
  std::vector<int> temp = x;
  temp.insert(temp.end(), y.begin(), y.end());
  
  //Iterate through temp array to populate result Set object
  for(int i = 0; i < temp.size(); i++)
  {
    result.add(temp[i]);
  }
  return result;
}

/* Overloads the & operator to produce a new Set object
 * that contains an intersection of the two original Set
 * objects
 */
Set operator&(Set& first, Set& second)
{
  Set result;
  std::vector<int> x = first.get_set();
  std::vector<int> y = second.get_set();
  std::vector<int> temp;

  for(int i = 0; i < x.size(); i++)
  {
    for(int j = 0; j < y.size(); j++)
    {
      if(x[i] == y[j])
      {
        temp.push_back(x[i]);

      }
    }
  }

  //loop through temp array to populate Set object
  for(int i = 0; i < temp.size(); i++)
  {
    result.add(temp[i]);
  }
  return result;
}

int main()
{
  Set a("A");
  Set b("B");
  Set c("C");

  a.add(1);
  a.add(2);
  a.add(5);
  a.add(7);
  a.add(7);
  a.add(7);
  a.add(3);
  a.add(1);
  a.add(420);

  b.add(3);
  b.add(7);
  b.add(69);
  b.add(9);
  b.add(5);
  b.add(5);
  b.add(22);
  b.add(22);
  b.add(3);

  c.add(22);
  c.add(7);
  c.add(5);
  c.add(9);
  c.add(73);
  c.add(69);
  c.add(69);
  c.add(69);
  c.add(420);
  c.add(69);

  std::cout << "Set " << a.get_name() << ": " << a;
  std::cout << "Set " << b.get_name() << ": " << b;
  std::cout << "Set " << c.get_name() << ": " << c;


  std::cout << "\nUnion of " << a.get_name() << " and " << b.get_name() << ": " << (a|b);
  std::cout << "Union of " << b.get_name() << " and " << c.get_name() << ": " << (b|c);
  std::cout << "Union of " << a.get_name() << " and " << c.get_name() << ": " << (a|c);

  std::cout << "\nIntersection of " << a.get_name() << " and " << b.get_name() << ": " << (a&b);
  std::cout << "Intersection of " << b.get_name() << " and " << c.get_name() << ": " << (b&c);
  std::cout << "Intersection of " << a.get_name() << " and " << c.get_name() << ": " << (a&c);

  //test out void Set::remove(int)
  a.remove(7);
  a.remove(420);
  b.remove(420);
  std::cout << "\nSet " << a.get_name() << " with '7' and '420' removed: " << a << std::endl;

  std::cout << "Set " << b.get_name() << " with '420' removed: " << b << std::endl;
	return 0;
}
Last edited on
coder777 (8439)
Are you aware that a std::set exists? See:

http://www.cplusplus.com/reference/set/set/?kw=set

Not only that the values are unique they are also ordered in a std::set.
Particularly your operator& might not generate correct result because the values are not ordered.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
#ifndef SET_H
#define SET_H
#include <iostream>
#include <vector>
#include <string>

class Set 
{
	public:
		Set() = default; // No implementation required
		Set(const std::string&); // Do not unnecessaryly copy data 
		void add(int);
		void remove(int);
                void print();
                const std::vector<int>& get_set() const; // Do not copy + const correctness
                const std::string& get_name() const;  // Do not copy + const correctness
                bool num_present(int, std::vector<int>); // This is not a member function. Consider make it static
	private:
		std::vector<int> num_set;
		std::string name;
};

#endif
I would suggest to put the prototypes of the operators in set.h and the implementation in set.cpp.
seeplus (6460)
- Pass std::string, std::vector et al as const ref rather than by value so a copy is not done. Eg

1
2
3
4
5
6
7
8
9
10
 
bool Set::num_present(int newNum, const std::vector<int>& set)
{
	bool found {};

	for (size_t i {}; !found && i < set.size(); ++i)
		if (set[i] == newNum)
			found = true;

	return found;
}


Also see other changes.

But why is vector passed at all - shouldn't this use the class vector?

1
2
3
4
5
6
7
8
9
10
 
bool Set::num_present(int newNum)
{
	bool found {};

	for (size_t i {}; !found && i < num_set.size(); ++i)
		if (num_set[i] == newNum)
			found = true;

	return found;
}



- For a constructor, use initialisation if possible rather than assignment. Eg:

 
 
Set::Set(const std::string& name_) : name(name_) {}


- If a member function doesn't change its member variables, mark it as const:

1
2
3
4
 
std::string Set::get_name() const
{
    return name;
}


Instead of using element number, it's sometimes better to use an iterator - eg in remove:

1
2
3
4
5
6
7
8
 
void Set::remove(int rNum)
{
	for (auto itr {num_set.begin()}; itr != num_set.end(); ++itr)
		if (*itr == rNum) {
			num_set.erase(itr);
			break;
		}
}


dakotad8218 (50)

Are you aware that a std::set exists? See:

http://www.cplusplus.com/reference/set/set/?kw=set

Not only that the values are unique they are also ordered in a std::set.
Particularly your operator& might not generate correct result because the values are not ordered.


Yes, I am aware that the standard library contains unordered and ordered sets. That was not the purpose of this assignment. The object was to overload the ostream operator, the &, and | to handle custom objects. Number sets were just a convenient way to demonstrate this principle that my teacher used as his worked example during lecture. This was the prompt for the assignment, verbatim:

Define a class Set that stores a finite set of integers. (In a set, the order of elements does not matter, and every element can occur at most once.) Supply add and remove member functions to add and remove set elements. Overload the | and & operators to compute the union and intersection of the set, and the
<< operator to send the set contents to a stream.


I would suggest to put the prototypes of the operators in set.h and the implementation in set.cpp.


We were specifically asked to not write these as member functions and to include them inline in main.cpp

Also thank you seeplus, I will have to implement a couple of those suggestions
Last edited on
Topic archived. No new replies allowed.