Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 ?

share|improve this question

2 Answers 2

up vote 6 down vote accepted

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).

share|improve this answer
4  
Never, ever store std::auto_ptr in a container. Further, auto_ptr is deprecated, so it shouldn't be used anyway. –  Yuushi Feb 13 at 1:49
    
@Yuushi Not even in a std::list? I imagined that list nodes would be immobile. –  ChrisW Feb 13 at 1:52
1  
"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. –  Yuushi Feb 13 at 1:53
    
Item will have subclasses and won't have any copy constructor. –  Michal Przybylowicz Feb 13 at 2:22
    
@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. –  ChrisW Feb 13 at 12:04

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.