0

I am writing a card game (poker) app as a learning project and have a question about deleting pointers. A bit of background first:

I have a class Player, which I want to initialize certain number of times, depending on user input. The constructor of class Player deals the cards, then checks the board (dealt in different class), checks what final hand player has etc. All cards are dealt on random (no user input).

I want this to happen in a while loop over and over again and display stats like who won, how many times, what hands were hit etc. So my main() does this: 

// GET NUMBER OF PLAYERS 
unsigned numOfPls = 1;
Player::getNumOfPls(numOfPls);

// CREATE POINTER TO numOfPls OBJECTS TYPE PLAYER
Player *p[numOfPls];
while(true){
    Deck::DeckObj().shuffleDeck();
    for (unsigned i=0; i<numOfPls; i++){
        p[i] = new Player;
    }

    // DO I DELETE THIS WITH EACH WHILE ??? 
    for (unsigned i=0; i<numOfPls; i++){
        //delete p[i];
    }
}
return 0;

I am not sure if I should delete p[i] each time. I'm reading "Sams Teach Yourself C++" where author says I should never reuse deleted pointers. But (not sure here) delete p[i] would not delete pointer, but only object created in array of object and pointed by p+i, correct?

On the other hand (if my above assumptions are correct) is it safe not to delete it and just overwrite object with another object created in another while ? What is the correct approach?

Also I guess it's OK not to delete *p as it will get destroyed with end of main()? (there will be some sort of "break" mechanism applied later).

dandan78
  • 13,328
  • 13
  • 64
  • 78
callmebob
  • 6,128
  • 5
  • 29
  • 46
  • or use smart pointers – Bryan Chen Oct 16 '14 at 06:16
  • The real question is whether you need to store pointers in the array, and if so, whether you need to use dynamic allocation. Most likely not, in which case you don't need to worry about memory management. – juanchopanza Oct 16 '14 at 06:17
  • 1
    I must point out, that if you call delete p[i] then p[i] will hold a pointer to a deleted object, which is often referred to as "deleted pointer" indeed, which you must not use. – Géza Török Oct 16 '14 at 06:22
  • 1
    Throw away the book. It is typical for a bad book to tell you to use raw arrays, raw owning pointers, and naked `new` and `delete`. A good book should've told you the ideas of scope, `std::vector`, smart pointers, RAII, etc. –  Oct 16 '14 at 06:50
  • @ Nicky C : Thanks for the advice. The book also covers std::vector, smart pointers etc. I just didn't get there yet and trying to understand what I learned so far. If you don't trust books, what alternative source of knowledge would you advise? – callmebob Oct 16 '14 at 06:58

3 Answers3

3

Yes, you should always use DELETE if you have used NEW :) But I do believe it is too complicated in your case.

Instead I may advise you make C++ take care of this or in other words - it is much easier to NOT use pointers in that particular case.

while(true){
  Player p[numOfPls];

  Deck::DeckObj().shuffleDeck();
  for (unsigned i=0; i<numOfPls; i++){
    // DO WHATEVER YOU WANT with the class, i.e. process data
  }

} //End of the loop
Ivailo Tanusheff
  • 81
  • 1
  • 4
1

There is a difference between reusing the storage (reusing the object) and allocating new memory. You should always pair new-delete. If you create new objects with new (reusing pointer slot in array, not the storage), you have to delete the old (valid) pointer first.

But you are in C++, use std::vector or std::deque ;)

...author says I should never reuse deleted pointers. That is about the storage, about dereferencing, about the memory the pointer is pointing to, not about the pointer itself:

Player* p = new Player();
delete p; // here p still has some value, but you should not dereference it
p->do_something(); // this is bad after the delete
p = new Player();  // p is valid again
p->do_something(); // ok
delete p; p = nullptr; // this is what you should do if you want to check it
if(p) p->do_something(); // won't execute now

Some more hints for you:

  1. Avoid new/delete in C++ if you can, rahter use std::unique_ptr or std::shared_ptr.
  2. Avoid container of pointers if you can, use container of objects - std::vector<Player> (most common), std::deque<Player> (when moving is heavy or not possible, preserves pointers to objects). Or std::array<Player,Size> if you know the size (alternatively Player[size]).
  3. If you have to, use container of smart pointers, e.g. vector<unique_ptr<Player>>, but check the above first (container of objects prefered).
firda
  • 3,268
  • 17
  • 30
  • Thanks, I'll look into vectors and deque - didn't get there yet in the book :) – callmebob Oct 16 '14 at 06:22
  • vector, deque, uniqupe_ptr or shared_ptr ;) `vector` if you can move them, `deque` if you need to preserve pointers (or copy is heavy), `vector>` as the alternative or `vector>` if you can work with the object outside of the vector (after removing, but not deleting it). – firda Oct 16 '14 at 06:26
  • @tx1131 The main point is now that type of container you use (you probably should use `std::vector`) but what you store in them. You probably don't want to be storing pointers to dynamically allocated objects. If you really need to, it would help to explain why in the question. – juanchopanza Oct 16 '14 at 06:29
1

Yes, you need to delete each object allocated with new, in the while loop. Otherwise, at next while iteration, the pointer array will be overwritten with new pointers, leaking the memory from the previous allocation.

You could also just declare a Player *p pointer and use array new for allocation:

p = new Player[numOfPls];

And array delete for destruction:

delete[] p;

For sure, there are better and safer ways to store an array of Player:

std::vector<Player> p(numOfPls);

Or if you really want pointers, use smart pointers which will be auto deleted:

std::vector<std::unique_ptr<Player> > p(numOfPls);
for(unsigned int i=0;i<numOfPls;i++) { p[i].reset(new Player()); }
galinette
  • 8,896
  • 2
  • 36
  • 87
  • So you are saying I should declare Player *p before while, then use it (with p = new Player[numOfPls]) inside while and delete [] p outside while? Isn't this going to leak exactly how you mention at the beginning of your answer? Note that Player class has to be created with each iteration as it's constrictor deals cards to players (as explained in the original post). As for vectors and smart pointers I will look into it. The above is the only way I learned so far as I'm quite new to this field. – callmebob Oct 16 '14 at 07:18
  • `new Player[numOfPls]` is allocating chunk (all objects in continuous storage), not array of pointers that you have to manually allocate. You can then delete them all at once (the whole chunk, the whole array of players, not array of pointers to players). This is major thing to understand if you come to C++ from e.g. C# or Java. – firda Oct 16 '14 at 07:22
  • @firda I understand that. But the option described above would mean I have to use new Player[numOfPls] in a loop without delete (if I delete in loop I will then use deleted pointer). This from what I just tested leaks memory. – callmebob Oct 16 '14 at 07:55
  • The above suggest to allocate and delete ***outside*** of the loop, the whole chunk, not one-by-one. That won't be leaking. You can as well reuse the array, if `numOfPls` won't change. This is all about understanding objects vs. pointers to objects. – firda Oct 16 '14 at 07:58
  • You can allocate and delete both **outside** the loop if you like. You can also allocate and delete both **inside** the loop. In both cases it will work. Just don't do one inside, the other outside. – galinette Oct 16 '14 at 10:11