C++ copy constructor, overload assignment operator , methos get()

Question

I had an exercise in my university in c++. So the course they asked me to make a copy constructor and an overload assignment operator = . So i did and it worked fine. They said me that i am wrong in methos get() and in my copy constructor and on overload assignment operator. Unfortunately the exercise worked fine. Also they didn't ask for main().

#include <iostream>

using namespace std;

class DIcourse{

private:
   int *ids;
   int numstudents;
   char *title;
   char *description;
public:
    DIcourse();
   ~DIcourse();
    DIcourse(const DIcourse &tmp);
    DIcourse operator=(const DIcourse &tmp);
    int const get_num();
    char const *get_title();
    char const *get_description();
    int const *get_ids();
    int set_num_ids(int num);
    char set_description(char *tmp);
    char set_title(char *temp);
};

DIcourse :: DIcourse(){
   ids=NULL;
   numstudents=0;
   title=new char[50];
   description=new char[200];
}

DIcourse :: ~DIcourse(){
   delete [] ids;
   delete [] title;
   delete [] description;
}

DIcourse :: DIcourse(const DIcourse& tmp){
   numstudents=tmp.numstudents;
   ids=new int[numstudents];
   for(int i=0;i<numstudents;i++){
      ids[i]=tmp.ids[i];
   }
   title=tmp.title;
   description=tmp.description;
}

DIcourse DIcourse::operator=(const DIcourse &tmp){
   delete [] ids;
   numstudents=tmp.numstudents;
   ids=new int[numstudents];
   for(int i=0;i<numstudents;i++){
      ids[i]=tmp.ids[i];
   }
   delete[] description;
   delete[] title;
   description= new char[200];
   title= new char[50];
   description=tmp.description;
   title=tmp.title;
   return *this;
 }

const int DIcourse :: get_num(){
  return numstudents;
} 

const char* DIcourse :: get_title(){
   return title;
}

const char* DIcourse :: get_description(){
    return description;
}

const int* DIcourse :: get_ids(){
   return ids;
}

int DIcourse :: set_num_ids(int num){
   numstudents++;
   int *temp_array;
   temp_array=new int[numstudents];
   temp_array[numstudents-1]=num;
   if(ids!=NULL){
     for(int i=0; i<numstudents-1; i++){
       temp_array[i]=ids[i];
    }
   delete [] ids;
   }
   ids=temp_array;
}

 char DIcourse :: set_description(char *tmp){
   int i=0;
   while(tmp[i]!='\0'){
       description[i]=tmp[i];
       i++;
   }
   description[i]='\0';
}

char DIcourse :: set_title(char *temp){
   int i=0;
   while(temp[i]!='\0'){
       title[i]=temp[i];
       i++;
   }
   title[i]='\0';
}

You should definitely read a [_good_ book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). This is very bad C++ code. It uses pointers all over the place and is exception-unsafe. — , Jun 12 '13 at 19:01
Reduce memory allocations, use `std::string` instead of `char *`. — Thomas Matthews, Jun 12 '13 at 19:11
@ThomasMatthews - As it is a University exercise I think learning to use dynamic memory allocation might be a worthwhile exercise. — Ed Heal, Jun 12 '13 at 19:18

taocp · Accepted Answer · 2013-06-12T19:16:32.167

Your copy assignment should be:

  DIcourse& operator=(const DIcourse &tmp);
 //^^return reference

Meanwhile, you should check for copy assignment operator self-assignment.

By courtesy of @Mooing Duck: you have memory leak in your current copy assignment operator implementation:

  DIcourse DIcourse::operator=(const DIcourse &tmp){
          delete [] ids;
          numstudents=tmp.numstudents;
          ids = new int[numstudents];
          for(int i=0;i<numstudents;i++){
              ids[i] = tmp.ids[i];
          }
          delete[] description;
          delete[] title;
          description= new char[200]; //^^
          title= new char[50]; //^^^memory leak here and above
          description=tmp.description; //^^this is not copy char*
          title=tmp.title; //^^this is not copy char* 
          return *this;
     }
 }

You may want to look at Copy-And-Swap Trick to implement copy assignment operator.

Your get functions should be:

int  get_num() const;
              //^^const modifier should be here, 
             //meaning that it access the value but does not change it

The const modifier problem is similar for your other getters.

@MooingDuck You are right. I have not looked into the implementation details. Will try to add now. — taocp, Jun 12 '13 at 19:03

score 1 · Answer 2 · answered Jun 12 '13 at 18:59

1

In addition to what taocp mentioned you also need to check in the assignment operator that you are not assigning to yourself.

i.e.

DIcourse &DIcourse::operator=(const DIcourse &tmp){

if (this != &tmp) {
   ... Do the business here

}
return *this;
}

answered Jun 12 '13 at 18:59

Ed Heal

59,252
17
87
127

C++ copy constructor, overload assignment operator , methos get()

2 Answers2