c++ list traversing and initialization

Question

I've been programming in managed code most of the time and I was interested in going back to C++. Been knocking my head all over the place(on Google) to find an answer. So I started doing the exercises here : http://www.cplusplus.com/forum/articles/12974/ and stumbled lots of errors. I tried the cola machine(second one) and it gave me the idea of making a cola machine and just tried to initialize a list in a Machine with pointers of Drink.

I didn't want to use the Boost library as I wanted to understand how containers worked(especially list).

I will post my code after the questions :

1) I am getting the following error in the line (*it)->getDrinkName() EXC_BAD_ACCESS on the method : getDrinkName() in Drink.cpp why is that? Am i not initializing correctly the list with the Drink?

When I try this:

Drink* test = new Drink("Coke");
cout << test->getDrinkName();

it works. Is it my constructor in Drink that is making it crash?

2) Am I suppose to initialize the list in the constructor of Machine? like :

_list = new list<Drink *>();

3) Here is the code :

Drink.h

#include <iostream>
#include <string>
using namespace std;

class Drink
{
public:
    Drink(string name);
    string getDrinkName();
private:
    string _name;
};

Drink.cpp :

#include "Drink.h"

Drink::Drink(string name)
{
    _name = name;
}

string Drink::getDrinkName()
{
    return _name;
}

Machine.h

#include <iostream>
#include <list>
#include "Drink.h"
using namespace std;

class Machine
{
public:
    Machine();
    list<Drink*> getList() const;
private:
    list<Drink*> _list;
};

Machine.cpp :

#include "Machine.h"

Machine::Machine()
{

}
list<Drink*> Machine::getList() const
{
    return _list;
}

main.cpp

#include <iostream>
#include <string>
#include "Machine.h"
using namespace std;

int main () {
    Machine* machine = new Machine();

    Drink* testCoke = new Drink("Coke");
    machine->getList().push_back(testCoke);
    std::list<Drink*>::const_iterator it ;
    for(it = machine->getList().begin();it!=machine->getList().end();it++)
    {
        cout << (*it)->getDrinkName();
        delete *it;
    }


    return 0;
}

Thanx in advance!

Answer 1

First of all, std::list should be just about the last priority among containers to learn about. Second, using dynamic allocation explicitly should be an even lower priority than that.

If I were going to simulate a soft-drink machine, it's a fair guess that you wouldn't find a single explicit pointer, new, delete, or iterator in anything I wrote. My first (admittedly simplified) version would probably look something like this:

#include <map>
#include <iostream>
#include <string>

int main() {
    // Stores a Drink and a quantity of that drink.
    // Establish initial stock according to drink quality.
    std::map<std::string, int> machine{ 
        { "Coke", 2 }, 
        { "Mt Dew", 97 }, 
        { "Diet Coke", 1 }
    };

    std::cout << "Please insert money and select from the following list:\n";
    for (auto const &s : machine)
        if (s.second > 0)
            std::cout << s.first << "\n";

    std::string temp;
    std::getline(std::cin, temp);
    while (machine.find(temp) == machine.end()) {
        std::cout << "\rBad name. Please a name from the list.";
        std::getline(std::cin, temp);
    }   
    --machine[temp];
    std::cout << "\nEnjoy your " << temp << "\n";   
}

At the risk of sounding condescending, your current code shows your background all too well. You start with the worst of ancient C++ practices, mix in the worst of "managed" code, and end up with an unreadable mess that barely runs limps along at all.

My advice would be that if you're going to try to write C++, rather than starting with a mindset like: "I"m going to use list and pointers", you instead start with: "what's the simplest and most effective way to solve this problem?" and act accordingly.

If your impression of that effective solution includes any raw pointers or uses of list, new or delete, you should probably stop right then and there, and do some more reading and/or thinking, because anything of the sort is a pretty solid indication that you probably don't know enough to solve the current problem well at all. If some more reading doesn't get you out of that hole, then chances are you're reading a lousy book--unfortunately, good books about C++ are nearly a rarity (you might want to check the C++ Book List for recommendations).

Answer 2

The problem is you are returning the container by value, not reference. machine->getList() makes a copy on every call, which goes out of scope in the for loop. Change it to:

const list<Drink*>& Machine::getList() const;

Edit: more explicit:

Lets look at this:

std::list<Drink*>::const_iterator it ;
for(it = machine->getList().begin();it!=machine->getList().end();it++)
{
    cout << (*it)->getDrinkName();
    delete *it;
}

The first call to machine->getList() creates an additional list. We call .begin() to get a pointer to the first element of that list. That list then goes out of scope, so it is destroyed: out pointer now points to deallocated memory. Our copy of the iterator (it) is now pointing to an invalid location. When we deference it (using *(it)), we get the error your seeing.

Answer 3

I would say, to find out, do this in your loop:

auto x = *it;
cout << x->getDrinkName();
delete x;

If you set a breakpoint on the auto x line, you should be able to see if x is equal to your previously defined testCoke. They should be the same.