Program crashes when adding object of user defined type to the STL vector

[cossay]

I am learning to using STL, and I'm currently on vectors. I have seen a lot of examples and practiced a lot on my own using built in types, but I'm finding very difficult storing objects that I create my self in a vector. Everything works fine when I add my objects through indexing (like an array), however, the program crashes whenever I add more than one object to the vector through the vector's push_back method.

This is my header file (student.h)

#ifndef STUDENT_H
#define STUDENT_H
class Student
{
    private:
        char * id;
        char * name;
        char * programme;
        int level;
    public:
        Student(const char * s_id = " ", const char * s_name = " ", const char * s_programme = " ", int s_level = 0);
        Student(const Student& student);
        ~Student();
        void setId(const char * s_id);
        void setName(const char * s_name);
        void setProgramme(const char * s_programme);
        void setLevel(int s_level);
        const char * getName() const;
        const char * getProgramme() const;
        const char * getId() const;
        int getLevel() const;
        bool operator<(const Student& student);
        bool operator==(const Student& student);
        Student& operator=(const Student& student);
};
#endif

and this is my implementation file (student.cpp)

#include <cstring>
#include "student.h"

Student::Student(const char * s_id, const char * s_name, const char * s_programme, int s_level)
{
    id = new char[strlen(s_id) + 1];
    name = new char[strlen(s_name) + 1];
    programme = new char[strlen(s_programme) + 1];
    strcpy(id, s_id);
    strcpy(name, s_name);
    strcpy(programme, s_programme);
}

Student::Student(const Student& student)
{
    setId(student.id);
    setName(student.name);
    setProgramme(student.programme);
    setLevel(student.level);

}

void Student::setId(const char * s_id)
{
    char * temp_id = new char[strlen(s_id) + 1];
    strcpy(temp_id, s_id);
    delete [] id;
    id = new char[strlen(temp_id) + 1];
    strcpy(id, temp_id);
    delete [] temp_id;
}

void Student::setName(const char * s_name)
{
    char * temp_name = new char[strlen(s_name) + 1];
    strcpy(temp_name, s_name);
    delete [] name;
    name = new char[strlen(s_name) + 1];
    strcpy(name, temp_name);
    delete [] temp_name;
}

void Student::setProgramme(const char * s_programme)
{
    char * temp_programme = new char[strlen(s_programme) + 1];
    strcpy(temp_programme, s_programme);
    delete [] programme;
    programme = new char[strlen(s_programme) + 1];
    strcpy(programme, temp_programme);
    delete [] temp_programme;
}

void Student::setLevel(int s_level)
{
    level = s_level;
}

const char * Student::getId() const
{
    return id;
}

const char * Student::getName() const
{
    return name;
}

const char * Student::getProgramme() const
{
    return programme;
}

int Student::getLevel() const
{
    return level;
}

bool Student::operator<(const Student& student)
{
    return strcmp(id, student.id) < 0 ? true : false;
}

bool Student::operator==(const Student& student)
{
    return strcmp(id, student.id) == 0 ? true : false;
}

Student& Student::operator=(const Student& student)
{
    if(this != &student)
    {
        setId(student.id);
        setName(student.name);
        setProgramme(student.programme);
        setLevel(student.level);
    }
    return *this;
}

Student::~Student()
{
    delete [] id;
    delete [] name;
    delete [] programme;
}

The program crashes whenever I try something like this

#include <iostream>
#include <vector>
#include "student.h"

using namespace std;

int main()
{
    vector<Student> students;
    Student obj("A123", "Kofi Mills", "BSc. (Information Technology), 400");
    students.push_back(obj);
    obj.setId("A124");
    students.push_back(obj);
    obj.setId("A125");
    students.push_back(obj);
    return 0;
}

[sepp2k]

Your copy constructor calls your setFoo methods. Your setFoo methods use delete on the old value of the variable that they're setting. When the copy constructor is invoked, the variables won't have old values because the object is brand new. That means that your setFoo methods will call delete on uninitialized pointers. This makes your program crash.

The quick fix would be to set your pointers to NULL in the copy constructor before you invoke your setter methods (deleting a null pointer is safe). A better solution would be to get rid of your char pointers altogether and use std::string instead.

[cossay]

Thank you Sepp2k, everything is working as expected now. I know using std::string will make things easier but, Like I said, I'm stilling learning so I'm just trying to make my self confortable with the char pointers just as I'm with thhe std::string.
Do think there are still other changes that I can make to my code to make it better? I'm waiting for response so I can mark this thread as solved.

[sepp2k]

booleanExpression ? true : false is just a more clumsy way of writing booleanExpression. Other than that (and not using std::string), I don't see anything that needs improvement.

Edit: On second thought you can simplify your setter methods by doing foo = temp_foo; after doing delete [] foo; instead of mallocing memory, strcpying and deleting a second time.

[cossay]

Thanks.