Operator Overload program from Herb Schildt c++ Beginner's Guide

[reprovo]

Hi.I've been studying Herb Schildt's c++ Beginner's Guide and have read a few(many) comments regarding errors and wrong practices.I'm currently on chapter 9,Operator Overloading.
I've added a small program (creating a Set Class) from the pdf book.
If It's not too much trouble , could someone point out any existing Issues with this code so I can get some Idea of what I may have been learning the wrong way.
Thank you.

#include <iostream>
using namespace std;

const int MaxSize = 100;

class Set {
 int len;  //number of members
 char members[MaxSize]; //this array holds the set

  /*The find() function is private because it
    is not used outside the Set class.*/
  int find(char ch); // find an element

public:

  //Construct a null set.
  Set() {len=0;}

  // Return the number of elements in the set.
  int getLength() {return len;}

  void showset(); //display the set
  bool isMember(char ch); //check for membership

  Set operator +(char ch); // add an element
  Set operator -(char ch); //remove an element

  Set operator +(Set ob2); // set union
  Set operator -(Set ob2); // set difference
};

/* Return the index of the element
   specified by ch,or -1 if not found. */
int Set::find(char ch) {
  int i;

for(i=0;i<len;i++)
  if(members[i] == ch) return i;

return -1;
}

//Show the set.
void Set::showset() {
 cout<<" { ";
 for(int i=0; i<len; i++)
 cout<< members[i]<< " ";

cout<< "}\n";
}

/* Return true if ch is a member of the set.
   Return false otherwise. */
bool Set::isMember(char ch) {
  if(find(ch) != -1) return true;
  return false;
}

//Add a unique element to a set.
Set Set::operator +(char ch) {
  Set newset;

  if(len==MaxSize) {
   cout<< "Set is full.\n";
   return *this; //return existing set
  }

  newset =*this; // duplicate the existing set
  
  // see if element already exists
  if(find(ch) == -1) { //if not found ,then add
    // new element to new set
   newset.members[newset.len] =ch;
   newset.len++;
  }
  return newset; // return updated set
}

// Remove an element from the set.
Set Set::operator -(char ch) {
  Set newset;
  int i = find(ch); // i will be -1 if element not found

// copy and compress the remaining elements
for(int j=0; j<len; j++)
 if(j != i) newset = newset + members[j];

return newset;
}

//Set union.

Set Set::operator +(Set ob2) {
 Set newset = *this; //copy the first set

// Add unique elements from second set.
for(int i=0; i< ob2.len; i++)
 newset =newset + ob2.members[i];

return newset; // return uupdated set
}

// Set difference.
Set Set::operator -(Set ob2) {
 Set newset = *this; //copy the first set

// Subtract elements from second set.
for(int i=0; i< ob2.len; i++)
 newset = newset - ob2.members[i];

return newset; // return updated set
}

// Demonstrate the Set class.
int main() {
 // construct 10-element empty Set
 Set s1;
 Set s2;
 Set s3;

s1=s1 + 'A';
s1=s1 + 'B';
s1=s1 + 'C';

cout << "s1 after adding A B C: ";
s1.showset();

cout<< "\n";

cout<<"Testing for membership using isMember().\n";
if(s1.isMember('B'))
 cout<< "B is a member of s1.\n";
else
 cout<< "B is not a member of s1.\n";

if(s1.isMember('T'))
 cout<<"T is a member of s1.\n";
else
  cout<<"T is not a member of s1.\n";
cout<<"\n";

s1=s1 - 'B';
cout<<"s1 after s1=s1 - 'B': ";
s1.showset();

s1=s1 - 'A';
cout<< "s1 after s1=s1 - 'A': ";
s1.showset();

s1=s1 - 'C';
cout<< "s1 after s1=s1- 'C': ";
s1.showset();

cout<<"\n";

s1=s1 + 'A';
s1=s1 + 'B';
s1=s1 + 'C';

cout<<"s1 after adding A B C: ";
s1.showset();

cout<<"\n";

s2=s2 + 'A';
s2=s2 + 'X';
s2=s2 + 'W';

cout<< "s2 after adding A X W: ";
s2.showset();

cout<<"\n";

s3=s1 + s2;
cout<< "s3 after s3=s1 + s2: ";
s3.showset();

s3=s3 - s1;
cout<< "s3 after s3 - s1: ";
s3.showset();

cout<< "\n";

cout<< "s2 after s2= s2 -s2: ";
s2=s2-s2; //Clear s2
s2.showset();

cout<<"\n";

s2=s2 + 'C';// add ABC in reverse order
s2=s2 + 'B';
s2=s2 + 'A';

cout<< "s2 after adding C B A: ";
s2.showset();

return 0;
}
 


  


 
  

[vlad from moscow]

Let consider for example the following declaration and the corresponding definition

Set operator +(char ch); // add an element

//Add a unique element to a set.
Set Set::operator +(char ch) {
  Set newset;

  if(len==MaxSize) {
   cout<< "Set is full.\n";
   return *this; //return existing set
  }

  newset =*this; // duplicate the existing set
  
  // see if element already exists
  if(find(ch) == -1) { //if not found ,then add
    // new element to new set
   newset.members[newset.len] =ch;
   newset.len++;
  }
  return newset; // return updated set
}

As it is seen the operator returns a new object of type Set. So you can not change the original object by using this operator. You will always create a new object. For example consider the code

Set s;

s + 'A';
s + 'B';
s + 'C';

s.showset(); // s is empty!

So it is obvious that you need at least one more operator as for example operator += that you can write

Set s;

s += 'A';
s += 'B';
s += 'C';

s.showset(); // { A B C }


Also it is not very well that the expression

'A' + s;

is not defined. Usually additive operations are commutative.

[vlad from moscow]

And I think you did not learn yet about const member functions. otherwise you shall use qualifier const with the most of the functions of the class and specify parameters as const references. For example

Set operator + ( const Set &ob2 ) const;

[Catfish4]

using namespace std;
You should unlearn that and instead prefix std:: where needed.
http://www.parashift.com/c++-faq/using-namespace-std.html

const int MaxSize = 100;
// ...
 int len;  //number of members 

Does it make sense for a size to be a negative number, like -30? Not really.

So why use a signed type for size? Do you plan on using "special values" like -1 to signal errors? This isn't the 1970's anymore.

/* Return the index of the element
   specified by ch,or -1 if not found. */
int Set::find(char ch) {
  int i;

for(i=0;i<len;i++)
  if(members[i] == ch) return i;

return -1;
}

Shit.

The alternatives that make sense are unsigned int or unsigned long int or std::size_t from cstddef. You should prefer the last because this is exactly what it's for!

#include <cstddef>

const std::size_t MaxSize = 100;
// ...
    std::size_t len; // number of members 

And when you can't find something, throw an exception.

#include <cstddef>
#include <stdexcept>

// ...

/* Return the index of the element
   specified by ch,or throw an exception if not found. */
std::size_t Set::find(char ch)
{
  for(std::size_t i=0; i < len; i++)
    if(members[i] == ch) return i;

  throw std::runtime_error("Cannot find ch.");
}

How to prevent your program blowing up with a runtime error when the set doesn't contain ch? Rewrite a proper Set::isMember() function that doesn't rely on Set::find().

Moving on, I see const correctness issues. More specifically, member functions that do not modify the data should be marked as const:
http://www.parashift.com/c++-faq/const-correctness.html

  // Return the number of elements in the set.
  int getLength() const
  {
    return len;
  }

  void showset() const; //display the set
  bool isMember(char ch) const; //check for membership 

Last but not least, the most difficult point: to properly implement a Set class you will not use a mere array. You will use some kind of tree data structure. A decent choice would be a Binary Search Tree. Professionals would probably use Red-black trees or AVL trees (or who knows what other newer, exotic kind).

But this last point is outside the scope of your thread. You want to become better at C++, data structure theory can wait.

[vlad from moscow]

@Catfish4

And when you can't find something, throw an exception.

 #include <cstddef>
#include <stdexcept>

// ...

/* Return the index of the element
   specified by ch,or throw an exception if not found. */
std::size_t Set::find(char ch)
{
  for(std::size_t i=0; i < len; i++)
    if(members[i] == ch) return i;

  throw std::runtime_error("Cannot find ch.");
}

It is a great stupidity to throw an exception from this simple private function.

[Catfish4]

It is a great stupidity to throw an exception from this simple private function.

All right, I admit I didn't notice it was private.

Please explain why throwing an exception is bad in this case, and say what's a better approach.

[vlad from moscow]

At least because such a simple method as

bool Set::isMember(char ch) {
  if(find(ch) != -1) return true;
  return false;
}

will not work and will throw an exception.

[Catfish4]

... which is why I suggested that Set::isMember() be rewritten.

So what's a better approach?

[reprovo]

Before I check all your answers ,I made a few errors when copying the program.I edited the program after vlad from moscow's Initial post.It may have affected his first answer. Sorry about that.
Thanks for the feedback!

[vlad from moscow]

@Catfish4

... which is why I suggested that Set::isMember() be rewritten.

So what's a better approach?

The better approach is do not take your advices.

[Catfish4]

The better approach is do not take your advices.

Ha ha! Uselessly confrontational. Anyway.

Back in the original Set::find() function, the return value has a double purpose. It gives the position when it's a positive value, and signals that the element wasn't found when it equals -1.

But since we are now using std::size_t, which for sizes is saner than signed int, this trick doesn't work anymore.

We could go back to the idea of double purpose, and return two values at the same time by using std::pair from the utility library.
http://www.cplusplus.com/reference/utility/pair/

#include <cstddef>
#include <utility>

std::pair<bool, std::size_t> find(char ch)
{
  std::pair<bool, std::size_t> r;

  r.first = false;

  for(std::size_t i=0; i < len; i++)
    if(members[i] == ch)
    {
        r.first = true;
        r.second = i;
        break;
    }

  return r;
}

If the bool is false, then the element wasn't found. Otherwise if the bool is true, we look at the size_t.

Edit: the function can be simplified by using std::make_pair():
http://www.cplusplus.com/reference/utility/make_pair/

#include <cstddef>
#include <utility>

std::pair<bool, std::size_t> find(char ch)
{
  for(std::size_t i=0; i < len; i++)
    if(members[i] == ch)
      return std::make_pair(true, i);

  return std::make_pair(false, 0);
}

[Cubbi]

could someone point out any existing Issues with this code so I can get some Idea of what I may have been learning the wrong way.

Wow, that's pretty bad indeed. It's especially dismaying to see so much wrong about operator overloading in what claims to be teaching that topic.

Catfish already pointed out quite a few problems, here's one more point of view

using namespace std;
fine in a small source file, very bad in a header file. Here it's up there before a class definition which would be something to go into a header file if you wanted to extend this.. so don't

const int MaxSize = 100;
part of Set, belongs inside the class, not outside. And, as Catfish said, it's silly for it to be a signed int

int len; //number of members
would work best as std::size_t as well, of course.

char members[MaxSize]; //this array holds the set
This makes each Set object occupy entire MaxSize bytes, even if it's "empty" or populated just by a handful of items as in this example. This should be std::vector<char> (which would also remove the need for MaxSize and len), or, given that these are chars, std::string. With a sorted vector members, it would even make it possible to turn this into an actually sensible example.

int find(char ch); // find an element
strange choice for a private member function, it has design problems (as already pointed out) - but even if you pretend that it is okay, it needs to be const.

Set() {len=0;}
This needs to be at least Set() : len(0) {} (or, for the fans of value-initialization Set() : len() {}). Of course, if members is a vector, it's not needed at all.

int getLength() {return len;}
needs to be const, if needed at all (is not used in this program)

void showset(); //display the set
needs to be const at least, but, more importantly, this should not be a class member function (although explaining interface design is not the topic here)

bool isMember(char ch); //check for membership
needs to be const (or possibly non-member)

Set operator +(char ch); // add an element
Set operator -(char ch); //remove an element 

I actually did a double-take here. Binary operators are (almost) never implemented as member functions. Also, if you're defining a binary operator, you need to define its compound assignment counterpart (operator+= for operator+, operator-= for operator-) , and *those* are often made member functions (although they don't need to be in general, they need to be here).

Set operator +(Set ob2); // set union
Set operator -(Set ob2); // set difference 

And (I think Catfish pointed it out too), this is needlessly taking the argument by value -- making an extra copy, and even if there's a chance for a move, since Set is using an array rather than a vector, it can't be optimized even then!

        int i;
        for(i=0;i<len;i++)

This isn't 1980s C, initialize the counter inside the loop, like in showset(). That is, if you want to use a loop at all - C++ has a std::find(), after all.

        if(find(ch) != -1) return true;
        return false;

A little redundant to evaluate a boolean expression and then return true if it is true, and the design is so questionable, but whatever, let's look at the operators.

       Set newset;
       [...]
        newset =*this; // duplicate the existing set 

what's the point of creating a Set if it's only given a value several lines (and possible return point) later? If you must do that, at least write Set newset = *this; (although it's bad for other reasons)

       // see if element already exists
        if(find(ch) == -1) { //if not found ,then add 

Did he forget he just defined isMember?

               cout<< "Set is full.\n";
                return *this; //return existing set 

operator+ shouldn't randomly print out messages to standard output and continue as if nothing happened, this calls for an exception (although if members were a regular, resizeable container, this wouldn't be a problem)

Now, how to rewrite those operator+'s using less awkward C++ (although it's still a bit painful because of the array involved)

class Set {

        [...]

        Set& operator+=(char ch) {
            if(len == MaxSize)
                throw std::overflow_error("Set is full in operator+");
            if(!isMember(ch))
                members[len++] = ch;
            return *this;
        }

        Set& operator+=(const Set& rhs) {
            for(int i=0; i< rhs.len; ++i)
                *this += rhs.members[i];
            return *this;
        }
};
// nonmember, one arg by value to call += on, other by ref
Set operator+(Set lhs, const Set& rhs)
{
    return lhs += rhs;
}
Set operator+(Set lhs, char rhs)
{
    return lhs += rhs;
}
Set operator+(char lhs, Set rhs) // so 'a' + set works too
{
    return rhs += lhs;
}

[vlad from moscow]

@Cubbi
And, as Catfish said, it's silly for it to be a signed int

int len; //number of members
would work best as std::size_t as well, of course.

The Straustrup book you advice usually to read for beginners throughout code examples uses type int for number of elements in a container So it seems you have double standards.:)

@Cubbi

Set operator+(Set lhs, const Set& rhs)
{
    return lhs += rhs;
}
Set operator+(Set lhs, char rhs)
{
    return lhs += rhs;
}
Set operator+(char lhs, Set rhs) // so 'a' + set works too
{
    return rhs += lhs;
}

Set operator+(const Set &lhs, const Set &rhs)
{
    return Set( lhs ) += rhs;
}
Set operator+(const Set &lhs, char rhs) 
{
    return Set( lhs ) += rhs;
}
Set operator+(char lhs, const Set &rhs)
{
    return Set( rhs ) += lhs;
}

[Catfish4]

What does that code accomplish, Vlad?

You pass by const reference only to make a copy, so it's the same thing, no?

[JLBorges]

vlad from moscow wrote:

it's silly for it to be a signed int
int len; //number of members
would work best as std::size_t as well, of course.

The nexc stupidy of beginner Framework stupid idiot vlad.
If you were not a beginner you would know that it is not rear that one project can be written in C++ and C# or other languages simultaneously. In this case it is very easy to make a wrong assumption because for example in C# this is a signed integer.

>

Set operator+(const Set &lhs, const Set &rhs)
{
    return Set( lhs ) += rhs;
}

Yet another stupidy of beginner stupid idiot vlad from moscow.

Do not repeat this this stupidity after the imbecile vlad from moscow.

[vlad from moscow]

@Catfish4

What does that code accomplish, Vlad?

You pass by const reference only to make a copy, so it's the same thing, no?

It is more effective to use const reference to an object when the object itself should not be changed in a function than to create a temporary copy of the object calling the copy constructor.
For example below is the definition of operator + for std::basic_string

template<class charT, class traits, class Allocator>
basic_string<charT,traits,Allocator>
operator+(const basic_string<charT,traits,Allocator>& lhs,
const basic_string<charT,traits,Allocator>& rhs);

You should follow this pattern.

[z05DSL3A]

reprovo, I feel I must apologise on behalf of the community. Maybe the people that should now better will take their petty squabbling and name calling elsewhere.

I have never bothered with Herb Schildt's books, mainly because he doesn't seem to have a good reputation. However the stuff people moan about is stuff that you don't learn straight of the bat (in most cases) but you learn from experience and further study.

[reprovo]

I'll take the good and leave the bad !
The one thing I've learnt Is that I've grasped a lot of core concepts from the book that have helped me understand (to a very small degree of course) how coding works. The wrong practices pointed out here don't make me feel like I've wasted my time In any way because I'm a total beginner.
These posts will be very helpful to me as I hopefully progress and I'll shift to some other literature recommended on this site.
Thanks for the Info and the time.

[vlad from moscow]

@reprovo

In fact I even can not call any good book on C++ for beginners. In my opinion for example the book "Programming. Principles and Practice using C++" by Straustrup is even worse than books of Schildt.
On the other hand there are many good books on C#.:) Though in most cases they are written for people who already has an experience in programming.