I have a problem with destructor in my c++ nested classes

Question

Here is my code. My teacher won't let us use string type, so I'm using pointers instead. When I'm debugging the program it says that the issue is in Author class destructor.

Thats the message I'm getting:

SIGTRAP (Trace/breakpoint trap)

It's occuring on delete[] name line.

I think it won't remember name field when I enter it, because it prints everything right instead of name which is just some random stuff.

#include <iostream>
#include <cstring>

using namespace std;

class Author{
private:
    char *name;
    char *surname;

public:
    Author();
    Author(char *, char *);
    Author(Author&);

    char* get_name();
    char* get_surname();

    Author& set_name(char *new_name);
    Author& set_surname(char *new_surname);

    void print_fields();
    void print();

    ~Author();
};

Author::Author() {
    this->name = new char[5];
    this->surname = new char[5];
    strcpy(name, "NONE");
    strcpy(surname, "NONE");
}

Author::Author(char *name, char *surname) {
    this->name = new char[strlen(name) + 1];
    this->surname = new char[strlen(surname) + 1];
    strcpy(this->name, name);
    strcpy(this->surname, surname);
}

Author::Author(Author &author) {
    this->name = new char[strlen(author.name)+1];
    this->surname = new char[strlen(author.surname)+1];
    strcpy(this->name, author.name);
    strcpy(this->surname, author.surname);
}

char* Author::get_name() {
    return this->name;
}

char* Author::get_surname() {
    return this->surname;
}

Author& Author::set_name(char *new_name) {
    delete name;
    this->name = new char[strlen(new_name) + 1];
    strcpy(this->name, new_name);
    return *this;
}

Author& Author::set_surname(char *new_surname) {
    delete surname;
    this->surname = new char[strlen(new_surname) + 1];
    strcpy(this->surname, new_surname);
    return *this;
}

void Author::print_fields() {
    cout << this->name << " " << this->surname;
}

void Author::print() {
    cout << "Author " << this->surname << endl;
}

Author::~Author() {
    delete[] name;
    delete[] surname;
}

class Book {
private:
    char *title;
    short code;
    Author author;
    short year;
    short number_of_pages;

public:
    Book();
    Book(char *, short, const Author&, short, short);
    Book(Book&);

    char* get_title();
    short get_code();
    Author get_author();
    short get_year();
    short get_number_of_pages();

    Book& set_title(char *new_title);
    Book& set_code(short new_code);
    Book& set_author(const Author& new_author);
    Book& set_year(short new_year);
    Book& set_number_of_pages(short new_number_of_pages);

    void print_fields();
    void print();

    ~Book();
};

Book::Book() {
    this->title = new char[5];
    strcpy(this->title, "NONE");
    this->code = -1;
    this->author = Author();
    this->year = 0;
    this->number_of_pages = 0;
}

Book::Book(char *title, short code, const Author& author, short year, short number_of_pages) {
    this->title = new char[strlen(title) + 1];
    strcpy(this->title, title);
    this->code = code;
    this->author = author;
    this->year = year;
    this->number_of_pages = number_of_pages;
}

Book::Book(Book &book): author(book.author) {
    this->title = new char[strlen(book.title) + 1];
    strcpy(this->title, book.title);
    this->code = book.code;
    this->year = book.year;
    this->number_of_pages = book.number_of_pages;
}

char *Book::get_title() {
    return this->title;
}

short Book::get_code() {
    return this->code;
}

Author Book::get_author() {
    return this->author;
}

short Book::get_year() {
    return this->year;
}

short Book::get_number_of_pages() {
    return this->number_of_pages;
}

Book& Book::set_title(char *new_title) {
    delete[] title;
    this->title = new char[strlen(new_title) + 1];
    strcpy(this->title, new_title);
    return *this;
}

Book& Book::set_code(short new_code) {
    this->code = new_code;
    return *this;
}

Book& Book::set_author(const Author& new_author) {
    this->author = new_author;
    return *this;
}

Book& Book::set_year(short new_year) {
    this->year = new_year;
    return *this;
}

Book& Book::set_number_of_pages(short new_number_of_pages) {
    this->number_of_pages = new_number_of_pages;
    return *this;
}

void Book::print_fields() {
    cout << this->title << " ";
    cout << this->code << " [";
    this->author.print_fields();
    cout << "] ";
    cout << this->year << " " << this->number_of_pages << endl;
}

void Book::print() {
    cout << "Book " << this->title << endl;
}

Book::~Book() {
    delete[] title;
    this->code = 0;
    this->year = 0;
    this->number_of_pages = 0;
}

int main() {
    Book book1;

    char *title = new char[255];
    char *name = new char[255];
    char *surname = new char[255];
    short code;
    short year, number_of_pages;

    cout << "Enter author\'s name:";
    cin >> name;
    cout << "Enter surname:";
    cin >> surname;
    Author author((char*) name, (char*) surname);

    cout << "Enter book\'s information:";
    cin >> title >> code >> year >> number_of_pages;
    Book book2((char*) title, code, author, year, number_of_pages);

    Book book3 = book2;

    book1.print_fields();
    book2.print_fields();
    book3.print();

    return 0;
}

I made char * fields const and Author(const Author&). The problem is still there although now it remembers the name field correctly.

After the program runs i get:

Process finished with exit code -1073740940 (0xC0000374)

and now it rewrites the name field in book1 with the name of book2.

This is how my console window looks like after the program ends:

Enter author's name:John
Enter surname:Brown
Enter book's information:Title 1 1999 200
NONE -1 [Title NONE] 0 0
Title 1 [John Brown] 1999 200
Book Title

Process finished with exit code -1073740940 (0xC0000374)

In the body of your question, please write the exact text of any error messages you are getting. When you say _"it says that the issue is in Author class destructor"_ that's not detailed enough. I think that potential answerers will prefer to see the exact error message which will potentially be quite familiar to them. — Wyck, Apr 21 '22 at 18:03
Stop messing around with `char*`, `new` and `delete`, there's `std::string`, `std::vector` and everything else you'll need for proper memory management. Who on earth told you to do it like in your code example?? — πάντα ῥεῖ, Apr 21 '22 at 18:03
You did not implement an assignment operator for `Author`. Your teacher is not only giving you instructions that are, in this day and age, not recommended for C++ programs, you were not told all the details necessary to create such classes as `Author` so that they can have correct copy semantics. — PaulMcKenzie, Apr 21 '22 at 18:03
@πάνταῥεῖ it literally says in the question that the teacher has forbidden use of a string type -- presumably this is a learning exercise in managing resources. (I fully concede that as a practical matter it's completely unnecessary in this case because of the existence of std::string) — Wyck, Apr 21 '22 at 18:05
*My teacher won't let us use string type* -- Create your own string class and use it. Which begs the question as to why your teacher simply didn't give you a string class to write -- you learn much more doing that then having one-off calls to `new[]` and `delete[]` strewn all over the place for `char *`. — PaulMcKenzie, Apr 21 '22 at 18:06
Can't use `string` is often secret code for "Write your own `string`. The folk who bash something that works together get a decent mark. The really good grades are reserved for the people who made a string class and wrote something that works around it. — user4581301, Apr 21 '22 at 18:10
`int main() { Author a1; Author a2; a2 = a1; }` -- That's all the code needed to demonstrate there are major problems with the `Author` class, even for simple code such as that. That code has a memory leak and a double-free error. — PaulMcKenzie, Apr 21 '22 at 18:13
what Unslander said, but in this case, rule of five. Author's assignment operator is the problem here. — Kenny Ostrom, Apr 21 '22 at 18:13
Lot of code there. It's a lot easier to find a bug when you have less code, so it's often a good idea to back up the code and see how much of it you can remove while still having the bug. It really helps if you write small amounts of code between tests because the bug is probably in the most recently added small amount of code, usually making it easier to isolate the problem. — user4581301, Apr 21 '22 at 18:14
IMHO, you should adopt a naming convention that differentiates members from parameters or local variables. Some guidelines recommend prefixing members with "m_", other guidelines recommend appending a "_" to the member names. Using these guidelines means you don't need to do the `this->` syntax. — Thomas Matthews, Apr 21 '22 at 18:14
I wonder if this is a question where the answer to fix the code calls for a rewrite of almost everything. That's the issue when you get questions from new C++ programmers who have been given these restrictions concerning using `std::string`, and the new programmer makes code where most if it has to be thrown away for things to work. — PaulMcKenzie, Apr 21 '22 at 18:16
Dont forget about the copy assignment operator. The compiler generated one does not play well with dynamically allocated pointers: `Author& operator=(Author const&)` — Martin York, Apr 21 '22 at 18:18
The rule of 3/5 is a must when your class manages dynamic allocated memory. I just discovered that this page https://en.cppreference.com/w/cpp/language/rule_of_three got some nice example added recently. It takes you all the way from rule of 3 to rule of 5 and finally rule of 0. Just because you are not allowed to use `std::string` does not mean that you are not allowed to encapsulate memory managment inside a class that does only that, and then use that class almost as you would `std::string` — 463035818_is_not_an_ai, Apr 21 '22 at 18:26
Rational for writing your own string class: Keep responsibilities of any one piece of code as small as possible. If you make one simple-as-possible object you can more easily verify that it correctly handles all of its responsibilities and then use it as a building block to make larger things. A [RAII-compliant](https://stackoverflow.com/questions/2321511) resource-management class confines responsibility for the resource to the class so that users of the resource have fewer responsibilities and can focus on doing the one thing they really need to do. — user4581301, Apr 21 '22 at 18:26
Also CHEAT LIKE HELL. You can't hand in code that uses `std string`, but you can write and test code that uses `std::string` so that you know you have your logic right. Then you write your string class, test it until you are sure it works and replace `std::string` with it. Do the same thing with `std::vector`, `std::list` and whatever else you need to grind out for classes. Try to keep the number of new things you have to write and test at any one time to an absolute minimum. — user4581301, Apr 21 '22 at 18:30
Does this answer your question? [What is The Rule of Three?](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — Stephen Newell, Apr 21 '22 at 19:05

Vlad from Moscow · Answer 1 · 2022-04-21T19:27:42.907

In these constructors of the class Book

Book::Book() {
    this->title = new char[5];
    strcpy(this->title, "NONE");
    this->code = -1;
    this->author = Author();
    this->year = 0;
    this->number_of_pages = 0;
}

and

Book::Book(char *title, short code, const Author& author, short year, short number_of_pages) {
    this->title = new char[strlen(title) + 1];
    strcpy(this->title, title);
    this->code = code;
    this->author = author;
    this->year = year;
    this->number_of_pages = number_of_pages;
}

you are using the default copy assignment operator of the class Author

    this->author = author;

The default copy assignment operator performs member-wise copying that results in undefined behavior.

You need to define explicitly the copy assignment operator for the both classes.

Also instead of the copy assignment operator used in bodies of the constructors you should place calls of constructors of the class Author in the mem-initializer list of the class Book.

For example

Book::Book() : Author() {
    this->title = new char[5];
    strcpy(this->title, "NONE");
    this->code = -1;
    this->year = 0;
    this->number_of_pages = 0;
}

But in any case you need to define the copy assignment operator explicitly.

And it is much better if the copy constructor will have its parameter as a constant reference. Also parameters that specify string also should be declared with the qualifier const. For example

Author( const char *, const char *);
Author( const Author&);

And casting pointer of the type char * to the type char * does not make a sense

 Book book2((char*) title, code, author, year, number_of_pages);
            ^^^^^^^^^^^^^

Also getters should have the type const char * and the corresponding member function should be constant member functions as for example

const char* Author::get_name() const {
    return this->name;
}

Otherwise the user of the class can change data members of the class incorrectly.

Also you have to use the operator delete[] instead of the operator delete for dynamically allocated character arrays like for example

Author& Author::set_name(char *new_name) {
    delete []name;
    ^^^^^^^^^^^^^
    //...

I changed everything how you said, but it still doesn't work, I added the issues at the end of the post. — Mykola Mateichuk, Apr 21 '22 at 19:00
@MykolaMateichuk See my appended answer. You shall use everywhere the operator delete [] instead of delete. Check all your functions. — Vlad from Moscow, Apr 21 '22 at 19:12

I have a problem with destructor in my c++ nested classes

1 Answers1