Understanding how to delete new dynamically allocated variables

[ninjachachi]

This code is to simulate a train where you start with an engine and you can add train cars, detach train cars and move between train cars. I have no problem with that but I have a problem with deleting all my new boxes that I have created while adding the trains at the end of the program. I want all my new train cars to remain until the user quits. At that point, I want to delete all the new boxes. How do I do that? I tried adding a destructor to my class with "delete next;" and "delete previous;" but the program had a run time error. I am new to pointers and dynamic memory and I want to know how to do that. Thanks very much!

  #include <iostream>
#include <string>

class train{
    public:
        train(string n);
        bool hasNext();
        bool hasPrevious();
        string getName();
        train* nextTrain();
        train* previousTrain();
        void add(string);
        void detach();
    private:
        string name;
        train* nextTrn;
        train* previousTrn;
};

void train::detach()
{
    if(previousTrn -> hasPrevious())
    {
        previousTrn = previousTrn -> previousTrn;
        delete previousTrn -> nextTrn;
        previousTrn -> nextTrn = this;
    }
    else
        delete previousTrn;
}

train::train(string n)
{
    name = n;
    nextTrn = NULL;
    previousTrn = NULL;
}

void train::add(string n)
{
    train* x = new train(n);
    if(hasPrevious())
    {
        x -> previousTrn = previousTrn;
        x -> previousTrn -> nextTrn = x;
    }
    previousTrn = x;
    x -> nextTrn = this;
}

train* train::nextTrain()
{
    return nextTrn;
}

train* train::previousTrain()
{
    return previousTrn;
}

bool train::hasNext()
{
    if(nextTrn != NULL)
        return true;
    else
        return false;
}

bool train::hasPrevious()
{
    if(previousTrn != NULL)
        return true;
    else
        return false;
}

string train::getName()
{
    return name;
}


int main()
{
    train engine = train("Engine");
    train* current = &engine;
    string choice;
    do
    {
        if(current -> hasNext())
        {
            cout << "Next train: " << current -> nextTrain() -> getName() << endl;
        }

        cout << "Current train: " << current -> getName() << endl;

        if(current -> hasPrevious())
        {
            cout << "Previous train: " << current -> previousTrain() -> getName() << endl;
        }

        cout << "Do you wish to go to the (n)ext train, (p)revious train, (a)dd a train, (d)etach a train, or (q)uit?\n";
        getline(cin,choice);

        if(tolower(choice[0]) == 'n' && current -> hasNext())
        {
            current = current -> nextTrain();
        }
        else if(tolower(choice[0]) == 'p' && current -> hasPrevious())
        {
            current = current -> previousTrain();
        }
        else if(tolower(choice[0]) == 'a')
        {
            cout << "Which train is this?\n";
            string name;
            getline(cin, name);
            current->add(name);
        }   
        else if(tolower(choice[0]) == 'd' && current -> hasPrevious())
        {
            current->detach();
        }           

    }while(tolower(choice[0]) != 'q');

    current = &engine;
    while(current -> hasPrevious())
    {
        current = current -> previousTrain();
        delete current -> nextTrain();
    }
    delete current;
}

[Golden Lizard]

Most likely you weren't deleting safely and tried to access/delete a dangling pointer. Your train acts like a doubly linked list, so you should consider using a temporary variable to hold the next value, while deleting the previous one.

~train()
{
   if(engine!=nullptr)
  {
   auto current=engine; //use this to iterate through your cars
   auto temp=current;   // use this to store next value

   while(current->next!=nullptr)
   {
        temp=current->next; // before you delete your value, store the next car, otherwise you won't be able to access it
        delete current; // delete car
        current=temp; //move forwards to the next car
   }
   
   if(temp) delete temp;
   temp=nullptr;
  }

};

[48T7M4Gy]

Without looking at your class and problem in minute detail I get the impression your data model would be better off if you differentiated between a train object and a list of trains and make them two separate classes.

Doing it this way makes it a lot clearer what adding and deleting a train means. deleting a train via a destructor is a lot different from removing a train from a list of linked trains while maintaining its existence for later or other use.

[ninjachachi]

I kind of made it unclear as to what the instructions are to do so I will put them below. For our assignment we were given a template that the course instructor coded and I was able to implement the adding of a train. This is what I'm having trouble with though:

Add functionality to detach the
previous train (the one behind the current train), if it exists. This time you need to ensure there are
no memory leaks and all “new”s are deleted. Any cars on the train that are left once the user quits
the loop should be deleted at the end of main().

[48T7M4Gy]

I think this answers your question. In short no 'new' means no 'delete' required.

http://stackoverflow.com/questions/4355468/is-it-possible-to-delete-a-non-new-object

[ninjachachi]

We were given a template that I had to modify, I was able to add the "add" functionality but with detach there is certainly a memory leak. I don't know how I would delete the 'new' that I have. We need to "ensure there are no memory leaks and all “new”s are deleted."

[ninjachachi]

Bump for more help!

[48T7M4Gy]

You need to be clear on what the purpose of your add method is, and why you need to have a new Train x in it when the constructor can do that.

A linkage of train objects can be achieved by each train having pointers to the next and previous trains as you have done. Those pointers would be to train objects created by the constructor outside the class rather than inside the class which creates something of a nightmare.

[ninjachachi]

The thing is, with this assignment, the first part in which you create the "add" function you are not allowed to change main(). The main we are given is

#include <iostream>

using namespace std;

int main()
{
	train engine = train("Engine");
	train* current = &engine;
	string choice;
	do
	{
		if(current -> hasNext())
		{
			cout << "Next train: " << current -> nextTrain() -> getName() << endl;
		}
		
		cout << "Current train: " << current -> getName() << endl;

		if(current -> hasPrevious())
		{
			cout << "Previous train: " << current -> previousTrain() -> getName() << endl;
		}
		
		cout << "Do you wish to go to the (n)ext train, (p)revious train, (a)dd a train, or (q)uit?\n";
		getline(cin,choice);
		
		if(tolower(choice[0]) == 'n' && current -> hasNext())
		{
			current = current -> nextTrain();
		}
		else if(tolower(choice[0]) == 'p' && current -> hasPrevious())
		{
			current = current -> previousTrain();
		}
		else if(tolower(choice[0]) == 'a')
		{
			cout << "Which train is this?\n";
			string name;
			getline(cin, name);
			current->add(name);
		}			
		
	}while(tolower(choice[0]) != 'q');

}

Everything we do has to make this main() work. The detach part which is what I'm having trouble with says nothing about keeping main the same, so if some of this looks coded weird, most likely it was because I was trying to get something to work with main.

[48T7M4Gy]

Well good luck with it.

If you can't modify main() in any way then someone might come along and have a neat answer to the way you are creating a train object in your add method instead of the constructor.

[ninjachachi]

I can modify main in the portion where I have to detach the train. Everything works for me, I just can't stop a memory leak