0

I am relatively new to classes and was introduced to copy constructors and overloading last week. I am supposed to overload the = operator and use it to assign multiple variables using the class name.

For some reason, running the program causes a popup saying

program.cpp has stopped responding.

I am positive there are minor/major things that I am missing due to me being a rookie with objects in C++.

Any advice is very much appreciated!

#include<iostream>
#include<string>

using namespace std;

class Employee
{
private:
    char *name;
    string ID;
    double salary;
public:
    Employee() {}
    Employee(char *name, string eid, double salary) {}

    Employee(const Employee &obj)
    {
        name = new char;
        ID = obj.ID;
        salary = obj.salary;
    }

    ~Employee() {}

    void setName(char *n)
    {
        name = n;

    }

    void setID(string i)
    {
        ID = i;
    }

    void setSalary(double s)
    {
        salary = s;
    }

    char getName()
    {
        return *name;
    }

    string getID()
    {
        return ID;
    }

    double getSalary()
    {
        return salary;
    }

    Employee operator = (Employee &right)
    {
        delete[] name;
        ID = right.ID;
        salary = right.salary;
        return *this;
    }

};

int main()
{
    Employee e1("John", "e222", 60000), e2(e1), e3, e4;

    e3 = e4 = e2;

    e2.setName("Michael");
    e2.setSalary(75000);
    e3.setName("Aaron");
    e3.setSalary(63000);
    e4.setName("Peter");


    cout << "\nName: " << e1.getName() << "\nID: " << e1.getID() <<     "\nSalary: " << e1.getSalary() << endl;
    cout << "\nName: " << e2.getName() << "\nID: " << e2.getID() <<     "\nSalary: " << e2.getSalary() << endl;
    cout << "\nName: " << e3.getName() << "\nID: " << e3.getID() <<     "\nSalary: " << e3.getSalary() << endl;
    cout << "\nName: " << e4.getName() << "\nID: " << e4.getID() <<     "\nSalary: " << e4.getSalary() << endl;

    return 0;
}
Paul Rooney
  • 20,879
  • 9
  • 40
  • 61
NacDan
  • 71
  • 1
  • 1
  • 7
  • What have you tried so far to diagnose the problem? – user253751 Mar 27 '17 at 00:19
  • 1
    You delete `name` and then don't allocate a new `name`. You can reuse the buffer, if the string is not too long. Otherwise you should allocate a larger one. The right way however is to use `std::string` and the that particular issue dissapears. You should also read [rule of three](https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)) – Paul Rooney Mar 27 '17 at 00:19
  • Also take heed of [compiler warnings](http://coliru.stacked-crooked.com/a/8f7aebb8262b2127). Set them to the maximum possible level at all times. – Paul Rooney Mar 27 '17 at 00:25
  • @PaulRooney I wasnt sure if I needed to put a function inside the brackets when allocating a new name inside the overloading operator. – NacDan Mar 27 '17 at 00:28
  • @immibis There were compilation errors before running the program so I tried correcting the overloading operator but I am unsure what to do when overloading a c-string pointer. I released memory then allocated new memory. Still contained problems. – NacDan Mar 27 '17 at 00:32
  • @NacDan there are quite a few issues. It might take a while to list them here. I would recommend reading a book, which will walk you through all this step by step. One very large hint though is your assignment operator should return a non const reference, as it is you are causing a copy on return, not what you wanted. – Paul Rooney Mar 27 '17 at 00:40
  • "overloading a c-string pointer" is a phrase that makes no sense. Functions or operators or electrical sockets can be overloaded, but not pointers. – user253751 Mar 27 '17 at 02:16

3 Answers3

1

There are several issues with this code.

The first issue is in the constructor Employee(char *name, string eid, double salary) {} which is just doing nothing and ignoring the passed data whereas it should be using it to initialize the fields (class member data).

Employee(char *name, string eid, double salary) 
{
    const size_t bufferSize = strlen(name) + 1; 
    this->name = new char[bufferSize];
    memcpy(this->name, name, bufferSize);

    this->ID = eid;
    this->salary = salary; 
}

The second issue is in the copy constructor Employee(const Employee &obj) , where you are just initializing the name (with single byte of char) and that's it. What the copy constructor suppose to do is initialize the fields (class members) of the class with the fields of the class object being passed to it.

Employee(const Employee &obj)
{
    const size_t bufferSize = strlen(name) + 1; 
    this->name = new char[bufferSize];
    memcpy(this->name, name, bufferSize);
    ID = obj.ID;
    salary = obj.salary;
}

the third issue is with the default constructor which is suppose to initialize the name pointer with the NULL so that the destructor could clean it up nicely:

Employee() : name(NULL) {}

~Employee() 
{
    if (NULL != name)
        delete[] name;
}

the fourth and last problem is with the assignment operator that's suppose to properly initialize the name member data instead of deleting it (which doesn't make sense)

Employee operator = (Employee &right)
{
    if (NULL != this->name)
        delete[] this->name;

    const size_t bufferSize = strlen(right.name) + 1; 
    this->name = new char[bufferSize];
    memcpy(this->name, right.name, bufferSize);
    ID = right.ID;
    salary = right.salary;
    return *this;
}
Mudi
  • 131
  • 7
  • 1) There is no need to check for NULL when calling `delete[]`. 2) The assignment operator is wrong in that it does not deallocate the old memory. – PaulMcKenzie Mar 27 '17 at 00:48
  • 1
    The copy constructor does not copy the name it only allocates the memory for it. The assignment operator also returns by value. Finally `NULL`? didn't you mean `nullptr` :) – Paul Rooney Mar 27 '17 at 00:56
  • agreed @Paul Rooney, made the change for copy constructor. nullptr is introduced in C++11 whereas NULL is there since the beginning :).. – Mudi Mar 27 '17 at 01:00
  • The assignment operation is still wrong and leaks memory. Once you fix it to return a `Employee&`, the fix is simple: `{Employee temp(right); std::swap(temp.name, name); std::swap(temp.ID, ID); std::swap(temp.salary, salary); return *this;}` – PaulMcKenzie Mar 27 '17 at 01:00
  • @PaulRooney Thanks for all the help! youve been here since the beginning haha!! Any books that i should stockpile in during the summer? – NacDan Mar 27 '17 at 01:11
  • sure http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list – Paul Rooney Mar 27 '17 at 01:12
  • @PaulMcKenzie Thank you for the advice :). I have had midterms since coming back and I haven't touched as much as I would have hoped to on this material. – NacDan Mar 27 '17 at 01:12
  • @Mudi Thank you! I am getting an understanding on the issues of the assignment operator. I liked how you implemented c-string functions. I can't remember many haha! – NacDan Mar 27 '17 at 01:17
  • @Mudi Slight change recommendation, `Employee operator =(Employee &right)` should be `Employee& operator =(Employee &right)` so that the reference `*this` is returned instead of an implicit copy of `Employee` (unless that's actually what you intended for some reason). – mr.stobbe Mar 27 '17 at 01:26
  • @Mudi Also, you might want to sanity check the copy-assignment op with `if (this != &right)`... but that's just for 5/5 stars. – mr.stobbe Mar 27 '17 at 01:27
0

The problem is this line:

        delete[] name;

You shouldn't delete anything you haven't allocated with new first. If you delete the above line, your program kind of works. :)

Sergey Slepov
  • 1,861
  • 13
  • 33
  • I removed it, but there it's still not responding. I debugged and it says that there is something on: char getName() { return *name; } – NacDan Mar 27 '17 at 00:40
-1

Here's a slightly revised version of your program that works:

#include<iostream>
#include<string>

using namespace std;

class Employee
{
private:
    char *name;
    string ID;
    double salary;
public:
    Employee() {}
    Employee(char *name, string eid, double salary)
        : name (name) // ADDED THESE
        , ID(eid)
        , salary(salary)
    {

    }

    Employee(const Employee &obj)
    {
        name = obj.name; // WAS: new char;
        ID = obj.ID;
        salary = obj.salary;
    }

    ~Employee() {}

    void setName(char *n)
    {
        name = n;
    }

    void setID(string i)
    {
        ID = i;
    }

    void setSalary(double s)
    {
        salary = s;
    }

    char * getName()
    {
        return name;
    }

    string getID()
    {
        return ID;
    }

    double getSalary()
    {
        return salary;
    }

    Employee operator = (const Employee &right)
    {
        name = right.name; // WAS: delete[] name;
        ID = right.ID;
        salary = right.salary;
        return *this;
    }

};

int main()
{
    Employee e1("John", "e222", 60000), e2(e1), e3, e4;

    e3 = e4 = e2;

    e2.setName("Michael");
    e2.setSalary(75000);
    e3.setName("Aaron");
    e3.setSalary(63000);
    e4.setName("Peter");


    cout << "\nName: " << e1.getName() << "\nID: " << e1.getID() << "\nSalary: " << e1.getSalary() << endl;
    cout << "\nName: " << e2.getName() << "\nID: " << e2.getID() << "\nSalary: " << e2.getSalary() << endl;
    cout << "\nName: " << e3.getName() << "\nID: " << e3.getID() << "\nSalary: " << e3.getSalary() << endl;
    cout << "\nName: " << e4.getName() << "\nID: " << e4.getID() << "\nSalary: " << e4.getSalary() << endl;

    return 0;
}
Sergey Slepov
  • 1,861
  • 13
  • 33
  • Setting `name (name)` during initialization of `Employee` is a really bad idea. It presumes that the pointer is references non-mutable information. It should be a copy. Likewise, setting `name = obj.name` in the copy op is just as bad for the same reasons. Keep going with `setName()` et al. Downvoted. – mr.stobbe Mar 27 '17 at 01:18
  • @NacDan It's **not** right. It's really bad and will segfault/stack overflow for sure. – mr.stobbe Mar 27 '17 at 01:20
  • I tested it and it doesn't segfault or stack overflow (why would it?). Add `const` to `char*` and you're good to go. – Sergey Slepov Mar 27 '17 at 01:24
  • I revised the constructor in my own way and it works correctly. Thank both of you all despite your disagreements with one another! – NacDan Mar 27 '17 at 01:40
  • It only works because all the strings are literals, which live for the entirety of the execution in the executables data segment. As soon as you switch to char buffers with shorter lifetimes, you will have issues. The employee objects should own the buffers they reference and presently they don't, asking for trouble. – Paul Rooney Mar 27 '17 at 01:47
  • @PaulRooney No... First, a string literal (as a constant) may be `const char* a = "hello world";` and `const char* b = "world";`. What do you think the compiler is allowed to do with that? Second, why in the world would you assume such a thing. Since when is it common to have an object called `Employee` take a property `name` that **is not** a variable? That's rather insane. – mr.stobbe Mar 27 '17 at 05:21
  • To @SergeySlepov It would because it's an `Employee` record meant to be mutable. E.g. the name should be **uniquely associated** with the object rather than hard coded in that data segment of the executable. I wonder what happens when you do `new Employee("Hello World", 1, 1000)`? Actually, C++ standards clearly indicate that it's undefined behavior (`-Wall` should give you warning). – mr.stobbe Mar 27 '17 at 05:28
  • @PaulRooney I rest my case. This answer is therefore trash because it copies a pointer and assumes the data pointed to is valid. – mr.stobbe Mar 27 '17 at 05:34
  • @PaulRooney Oh, **sigh** I just realized I was arguing against nothing. Sorry was juggling multiple things. Yes, looking back at the original comment, we absolutely agree. I apologize for the mishap. Copying pointers is bad. Everyone can agree one that :). – mr.stobbe Mar 27 '17 at 05:46