Using dynamically allocated c-style strings in a class with copy and assignment operator giving unexpected results

Question

This program compilation succeeded but it doesn't work. I guess it has something to do with the assignment operator or the copy constructor but I can't figure out what...

Header:

class employee{
    char *name;
    unsigned int salary;
public:
    employee();
    employee(const char*);
    employee(const char*,unsigned int);
    employee (const employee&);
    employee operator = (employee);
    void init(const char*,unsigned int);
    void print();
    ~employee();
};

Cpp:

#include <iostream>
#include <string>
#include "class.h"

using namespace std;

employee::employee() : salary(1000)
{
    name=new char[20];
}

employee::employee(const char* ename) : salary(1000)
{
    strcpy_s(name,20,ename);
}
employee::employee(const char* ename,unsigned int salary)
{
    name=new char[20];
    strcpy_s(name,20,ename);
    this->salary=salary;
}

employee::employee(const employee& emp)
{
    name=new char[20];
    int i=0;
    while (name[i]!='\0') 
    {
        name[i]=emp.name[i];
        i++;
    }
    salary=emp.salary;
}

void employee::init(const char* ename, unsigned int salary)
{
    name=new char[20];
    strcpy_s(name,20,ename);
    this->salary=salary;
}

void employee::print()
{
    cout<<"name: ";
    int i=0;
    while (name[i]!='\0')
    {
        cout<<name[i];
        i++;
    }
    cout<<"\n"<<"salary: "<<salary<<endl;
}

employee employee::operator = (employee emp)
{
    strcpy_s(name,20,const_cast <const char*>(emp.name));
    emp.salary=salary;
    return *this;
}

employee::~employee()
{
    delete [] name;
}

Main:

#include <iostream>
#include "class.h"

using namespace std;


int main()
{
    employee emp1 ("Bill Jones",5000),emp5("Michael Adams");
    employee emp2;
    emp2=emp1;
    employee emp3;
    emp3=emp2;
    employee * workers= new employee [3];
    workers[0]=emp3;
    workers[1]= employee("katy Ashton");
    delete [] workers;
}

Welcome to SO. Please read [help] and [ask] docs. Posting a bunch of code, and saying "it doesn't work" is not going to get you a happy response. — OldProgrammer, Feb 04 '14 at 15:25
employee::employee(const char* ename) : salary(1000) { strcpy_s(name,20,ename); } this is the problem allocate name first — Hayri Uğur Koltuk, Feb 04 '14 at 15:26
you are not outputting anything, what do you expect on running it? — Stephen Jacob, Feb 04 '14 at 15:27
I would recommend you use `std::string` over your current approach. — , Feb 04 '14 at 15:27
Agreed re: `std::string`. Why are you bothering with C-strings? — cHao, Feb 04 '14 at 15:37

score 2 · Answer 1 · edited May 23 '17 at 12:11

While you may be doing this as an exercise, I recommend you read up on What is the Rule of Three? It and similar FAQs will guide you on how to properly overload copy/assignment operators and writing your own copy constructors and handle dynamic memory appropriately. For now, to avoid error-prone code, I suggest you use std::string and forgo overloading entirely. Your new class should look like this:

#include <iostream>

using namespace std;

class employee{
    std::string name;
    unsigned int salary;
public:
    employee();
    employee(std::string);
    employee(std::string,unsigned int);

    void init(std::string,unsigned int);
    void print();
    ~employee();
};
employee::employee() : salary(1000)
{
    name = "";
}

employee::employee(std::string ename) : salary(1000)
{
    name = ename;
}
employee::employee(std::string ename,unsigned int salary)
{
    name = ename;
    this->salary=salary;
}

void employee::print()
{
    cout<<"name: "<<name;
    cout<<"\n"<<"salary: "<<salary<<endl;
}

employee::~employee()
{

}


int main()
{
    employee emp1 ("Bill Jones",5000),emp5("Michael Adams");
    employee emp2;
    emp2=emp1;
    employee emp3;
    emp3=emp2;
    employee * workers= new employee [3];
    workers[0]=emp3;
    workers[1]= employee("katy Ashton");
    for (int i = 0; i < 2; i++)
    {
        workers[i].print();
    }
    delete [] workers;
}

This is actually a better case for the [Rule of Zero](http://flamingdangerzone.com/cxx11/2012/08/15/rule-of-zero.html). :) One should try to avoid having copy/move constructors/assignment operators/destructors at all, if the defaults can do the job. — cHao, Feb 04 '14 at 15:47

score 1 · Answer 2 · edited Feb 04 '14 at 15:42

1

I believe that this part is wrong:

while (name[i]!='\0') 
    {
        name[i]=emp.name[i];
        i++;
    }

the while-condition should be emp.name[i]!='\0'. (consider using strcpy_s, which does the same thing.)

Another problem is in your employee::employee(const char* ename) constructor, where you copy into array, which is not allocated. You need to allocate it first:

employee::employee(const char* ename) : salary(1000)
{
    name = new char[20]; // typo! it was 10... better use defines in this situation!
    strcpy_s(name,20,ename);
}

Of course, you should describe your problem more precisely.

edited Feb 04 '14 at 15:42

BeerBaron

388
6
18

answered Feb 04 '14 at 15:29

vhavel

146
1
6

Might as well use `strcpy_s`, considering it's being used elsewhere. – cHao Feb 04 '14 at 15:36
Thanks cHao, you are right. I've changed it for strcpy_s. – vhavel Feb 04 '14 at 15:37

score 1 · Answer 3 · answered Feb 04 '14 at 15:30

1

I think you are missing name = new char[20]; in the employee constructor that takes just the name parameter

answered Feb 04 '14 at 15:30

BeerBaron

388
6
18

score 1 · Answer 4 · answered Feb 04 '14 at 15:31

I don't know what the error message is, but I guess the reason is:

employee::employee(const char* ename) : salary(1000)
{
    strcpy_s(name,20,ename);
}

you don't allocate any space for name here and you use this constructor later: ,emp5("Michael Adams");

Therefore it crashes (I guess)

Using dynamically allocated c-style strings in a class with copy and assignment operator giving unexpected results

4 Answers4