Inserting new Object to Repository throws error

Question

I've created an object and a Repository for it. When I try inserting the object into the Repository (with the insert function i've created) I get compile error.

The Class I'm trying to insert into Repository

class Payment{
private:
    int day;
    int amount;
    char *type;

public:
    Payment();
    Payment(int day, int amount, char *type);
    Payment(const Payment &p);
    ~Payment();

    //getters
    int getDay()const;
    int getAmount()const;
    char* getType()const;

    //setters
    void setDay(int day);
    void setAmount(int amount);
    void setType(char* type);

    //operator
    Payment& operator=(const Payment& other);
    friend ostream& operator<<(ostream &os,const Payment &obj);
};

//copy constructor
Payment::Payment(const Payment & p){
    this->day = p.day;
    this->amount = p.amount;
    if(this->type!=NULL)
        delete[] this->type;
    this->type = new char[strlen(p.type)+1];
    strcpy_s(this->type, strlen(p.type) + 1, p.type);
}

//assignment operator
Payment& Payment::operator=(const Payment &other) {
    this->day = other.day;
    this->amount = other.amount;
    this->type = new char[strlen(other.type) + 1];
    strcpy_s(this->type, strlen(other.type) + 1, other.type);
    return *this;
}

//destructor
Payment::~Payment(){
    this->day = 0;
    this->amount = 0;
    if (this->type != NULL) {
        delete[]this -> type;
        this->type = NULL;
    }
}


//Repository header
class Repository{
private:
    vector<Payment> list;
public:
    Repository();

    int getLength();

    void insert(const Payment& obj);
    void remove(int position);
};

//Repository cpp
Repository::Repository(){
    this->list.reserve(10);
}

//return the size of the list
int Repository::getLength() {
    return this->list.size();
}

//add payment to list
void Repository::insert(const Payment &obj) {
    this->list.emplace_back(obj);
}

//remove payment from list
void Repository::remove(int position) {
    this->list.erase(this->list.begin() + position);
}

In main function I have

char c[] = "some characters";
Payment pay = Payment(7,9,c);
Repository rep = Repository();
rep.insert(pay);

When I run the program I get the error " Expression: _CrtlsValidHeapPointer(block) "

That is not a compiler error, that is a runtime error. Second, why are you not using `std::string type;`? That would have solved your problem. If not that, this is a duplicate of how to implement the "rule of 3", i.e. you are missing a user-defined copy constructor for `Payment`. — PaulMcKenzie, Mar 24 '19 at 15:51
The copy constructor may solve the problem. I completely forgot about it. I'll try it and leave a reply with the result. Thanks for the help! ^_^ — Gimme your company, Mar 26 '19 at 12:27
Please post your code that implements the copy constructor, assignment operator, and destructor. That is where the problem is. A `std::vector` requires that the type you're placing in a vector has correct, non-buggy, copy semantics. If you had used `std::string` instead of `char *`, then this wouldn't be an issue. Since you insist on using `char *`, then it is up to you to now code all of those functions, without error. — PaulMcKenzie, Mar 27 '19 at 12:49
Also, please see [the rule of 3](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — PaulMcKenzie, Mar 27 '19 at 12:55
@PaulMcKenzie I also created the copy constructor but still doesn't work. I wanted to use dynamic allocation. Wouldn't that be faster than using `std::string`? You're suggesting me to use `std::string`, so when I should use `string` and when I should use `char` type? Even if I change the type, I'm dying to know how to get over this error. — Gimme your company, Mar 27 '19 at 12:58
*Wouldn't that be faster than using std::string?* -- Why do you think it would be faster? If anything the difference is negligible -- not only that, you've been trying to get this to work for 3 days, when you could have solved the issue in 3 minutes.. Second, a `std::string` is a string. Isn't that what you're trying to do? Third, put the updated code in the post, not in the comment section. — PaulMcKenzie, Mar 27 '19 at 13:02
Your copy constructor and assignment operator are totally incorrect. Why are you calling `delete[]` inside of the copy constructor? A copy constructor is creating a *brand new* object, so what is there to call `delete[]` on? Second, your assignment operator is totally incorrect for several reasons. Last, your destructor -- why are you doing all of that work when all you need to do is `delete [] type;`? Again, you wouldn't need to write **any** of these functions if you used `std::string type;`. — PaulMcKenzie, Mar 27 '19 at 13:15
@PaulMcKenzie My school teacher is obsessed with `char*` and they always want us to use it instead of `std::string`. I would have used `string` a long time ago. — Gimme your company, Mar 27 '19 at 13:17
Well, if that's the case, why didn't your teacher show you an example of how to correctly write a class that has correct copy semantics? Why does it take someone from StackOverflow to show this (when the teacher should have shown you examples of it)? — PaulMcKenzie, Mar 27 '19 at 13:18
I posted an answer -- I leave it to you to carefully study the links I gave you. Also, `std::string` has been part of C++ for over 20 years, so it makes no sense to me to use `char *`, unless you are being taught to write your own string class. — PaulMcKenzie, Mar 27 '19 at 13:28

PaulMcKenzie · Accepted Answer · 2019-03-27T17:26:28.103

Since std::vector will make copies, a std::vector<Payment> requires that Payment has correct copy semantics. Your copy constructor and assignment operator are not implemented correctly. The assignment operator causes memory leaks, since you failed to delete [] the existing memory.

The easiest solution is to drop using the char *type; member and simply use std::string type;. Then the Payment class would have the correct copy semantics automatically.

Given that, the corrections to your Payment class is below:

#include <algorithm>
//...
Payment::Payment() : day(0), amount(0), type(nullptr) {}

Payment::Payment(const Payment & p) : day(p.day), amount(p.amount), type(nullptr)
{
    if ( p.type )
    {
        type = new char[strlen(p.type) + 1];
        strcpy_s(this->type, strlen(p.type) + 1, p.type);
    }
}

// Use the copy/swap idiom    
Payment& Payment::operator=(const Payment &other) 
{
    Payment temp(other);  // make a temporary copy
    // swap out contents of temporary with this object
    std::swap(temp.day, day);  
    std::swap(temp.amount, amount);
    std::swap(temp.type, type);
    return *this;  
 }  // when this brace has been reached, the temp copy dies off with the old data

Payment::~Payment()
{
   delete [] type;
}

The above uses the copy/swap idiom within the assignment operator. The copy constructor uses a member initialization list.

The destructor need not check for a null pointer, since deleting a nullptr is perfectly valid.

score 0 · Answer 2 · answered Mar 27 '19 at 14:14

Now adding to a std::vector is working well, without any runtime error (using the code @PaulMcKenzie posted). I've also found an example of code that's working, where only the assignment operator is kinda different. Converted to my code would be (and it's working either):

Payment& Payment::operator=(const Payment &other) {
    if (this != &other) {
        this->setDay(other.day);
        this->setAmount(other.amount);
        this->setType(other.type);
    }
    return *this;
}

Thanks for the help! Now it's working perfectly! I didn't get to study very much from <algorithm> library, so I'll have to take a closer look. Wish you the best luck! ^_^

This is not correct. Your assignment operator has a memory leak. If you read my answer, I mentioned that your old version leaked memory. There is a reason for `std::swap` in my answer, and it isn't there just for decoration. Again, please read up on what copy / swap is and why it works. — PaulMcKenzie, Mar 27 '19 at 17:23

Inserting new Object to Repository throws error

2 Answers2