8
\$\begingroup\$

I am writing small roguelike game where I spawn some items and put them into the std::list. Is this the right way to do it?

The Item class is as followed (I know it has public id property):

class Item {

public:
   Item();
   ~Item();

   unsigned int id;
};

Somewhere in the code I also defined:

list<Item*> listOfItems;
list<Item*>::iterator it;

Now I am using this code to create an item:

Item *p = new Item();
p->id = itemCrowbar;
listOfItems.push_back(p);

At the end of the program I use small loop to delete all items:

for (it = listOfItems.begin(); it!=listOfItems.end(); it++) {
    delete *it;
}

Is this approach for creating objects, putting them in std::list and at the end deleting them correct, or am I missing something important here?

\$\endgroup\$
0

2 Answers 2

Reset to default
7
\$\begingroup\$

If Item has subclasses, then make the ~Item() destructor virtual.

If Item has a copy constructor, and is not a superclass of a subclass, then you can store a copy of the item (not pointer to the item) in the list:

list<Item> listOfItems;

Item item;
item.id = itemCrowbar;
listOfItems.push_back(item);

In that case, you won't need to delete Item pointers before the list is destroyed.

However, if Item has subclasses, then this might be legal but a bug (called 'object slicing'):

list<Item> listOfItems;

SubclassOfItem item;
item.id = itemCrowbar;
item.extra = "hello";
listOfItems.push_back(item);

Or you can create a list of 'smart pointers' (std::unique_ptr) to items: which behave like pointers to Items except that you don't need to delete the Item (the Item is deleted when the smart pointer which contains it is destroyed).

\$\endgroup\$
9
  • 4
    \$\begingroup\$ Never, ever store std::auto_ptr in a container. Further, auto_ptr is deprecated, so it shouldn't be used anyway. \$\endgroup\$ Commented Feb 13, 2014 at 1:49
  • \$\begingroup\$ @Yuushi Not even in a std::list? I imagined that list nodes would be immobile. \$\endgroup\$ Commented Feb 13, 2014 at 1:52
  • 1
    \$\begingroup\$ "Copying an auto_ptr copies the pointer and transfers ownership to the destination: both copy construction and copy assignment of auto_ptr modify their right hand arguments, and the "copy" is not equal to the original. Because of these unusual copy semantics, auto_ptr may not be placed in standard containers." See here. \$\endgroup\$ Commented Feb 13, 2014 at 1:53
  • \$\begingroup\$ Item will have subclasses and won't have any copy constructor. \$\endgroup\$ Commented Feb 13, 2014 at 2:22
  • \$\begingroup\$ @MichalPrzybylowicz Even without a copy-constructor there might (in modern C++) be a move constructor; but if there are several subclasses then you're right that you need to store by pointer, not by value. Don't forget to make your destructor virtual. And consider using a mart pointer to avoid the need to do explicit destruction. \$\endgroup\$ Commented Feb 13, 2014 at 12:04
1
\$\begingroup\$

No, it is not correct.

For such classes, you should do not need to create objects on the heap. This should do :

Item p;
p.id = itemCrowbar;
listOfItems.push_back(p);

If you add a constructor with a parameter like this ...

class Item {

public:
    Item(unsigned int id_):id(id_){}

   unsigned int id;
};

... then you could even do this :

int main()
{
    std::list<Item> items;

    unsigned int someId = 5;

    items.push_back( {someId} );
}

This way you do not have to wory about memory leaks, and you can remove the loop releasing created objects.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.