Pointer Help in C++

Question

I am new to c++(have a java background) and thus pointers are sort of new to me. I am dealing with an array of pointers where each index points to an object on the heap as so:

Deck::Deck()
{
seed = rand()%100; //this will be used in shuffle method
srand(seed); 
for(int i=0;i<deckSize;i+=3) //deckSize=12 in this case, p defined as CardTypes* p[deckSize]
{
    p[i]= new Infantry(); 
    p[i+1] = new Artillery();
    p[i+2] = new Cavalry();

}

} All 3 of these classes are subclasses of the class CardTypes(which was only created so I could store diff types in an array).

class CardTypes
{
public:
virtual string getCard() = 0;
virtual ~CardTypes() {};
};
class Infantry: public CardTypes
{
const string name = "Infantry";
public:
string getCard(); //this simply returns "name" so that I can differentiate each object in the array by a data value
};
class Artillery:public CardTypes 
{
const string name= "Artillery";
public:
string getCard();
};
class Cavalry:public CardTypes
{
const string name = "Cavalry";
public:
string getCard();
};

Although not a great way to do it, I have created another array of pointers(CardTypes* s[deckSize) which copies pointers from p into s randomly(thus mimicking a shuffle in a deck of cards):

void Deck::shuffle() //this is the method that puts objects in s to be grabbed in draw()
{
int j = 0;
int k = 1;
int l = 2; //initial setup(index 0 will have Infantry, index 1 will have Artillery and index 3 will have Cavalry and this pattern continues throughout p)
int n = rand()%3 + 1; //gives random # between 1 and 3 1=infantry,2 = artillery,3 = cavalry
int i=0; //counter for loop
while(i<deckSize)
{
     n = rand()%3+1;
    if(n==1)
    {
        if(j>9) //means no more infantry cards as due to pattern of p 
        infantry cards stop after index 9
        {
            continue; //used to reset loop foranother iteration(will get random number,I know this is bad for time complexity)
        }
        else
        {
        s[i] = p[j]; //copy "Infantry" pointer to s
        j+=3;
        i++;
        }
    }
    else if(n==2)
    {
        if(k>10)//means no Artillery cards due to pattern in p
        {
            continue;
        }
        else
        {
            s[i] = p[k];//copy "Artillery" pointer to s
            k+=3;
            i++;
        }
    }
    else
    {
        if(l>11) //means no more cavalary cards due to pattern in p
        {
            continue;
        }
        else
        {
            s[i] = p[l]; //copy "Cavalry" pointer to s
            l+=3;
            i++;
        }
    }
}
}

Now my issue is i am trying to create a draw method that grabs a pointer from s and returns it. My program completely crashes when I attempt this and I am not sure why:

CardTypes* Deck::draw() //draws a card from the deck and returns it
{
CardTypes* card = s[deckSize];
delete s[deckSize];//clear heap
s[deckSize] = NULL;//remove what pointer was pointing too (as card has been drawn)
deckSize--;
return card;
}

I then attempt to call this method:`

int main()
{
Deck d1;
d1.shuffle(); //this works
d1.getCurrentDeck();//this works, just prints out each objects getCard() method in s
CardTypes* card = d1.draw();//does not cause a crash
cout<<"Card:"<<card->getCard() <<"\n";//crashes here
}

This issue is probably due to my inexperience with pointers but any help would be appreciated. Also note I delete the arrays after I am done with the program using delete [] p and delete [] s, I have not included this in the code as it is not of issue right now.

Answer 1

Your problem is you are deleting the instance and, after that, you want to use it.

You are creating many instances:

for(int i=0;i<deckSize;i+=3) //deckSize=12 in this case, p defined as CardTypes* p[deckSize]
{
    p[i]= new Infantry(); 
    p[i+1] = new Artillery();
    p[i+2] = new Cavalry();

}

The size of your array is determined by the variable deckSize.

In your function Deck::draw() your have some errors:

• You are set a pointer of an instance of CardType with this code: CardTypes* card = s[deckSize]; But the array s has an index base 0, so s[deckSize]is accessing another memory sector that is not assigned to array s (Could a Memory Access Violation). use s[deckSize-1] instead of s[deckSize]..

• You are release the memory that was assigned to pointer card and this pointer is returned to be used outside the function, which try to use this instance but is doesn't exist any more. So the memory which card and s[deckSize] share is released. Don't forget that s[deckSize] could raise a Memory Access Violation.

Check your code:

CardTypes* Deck::draw() //draws a card from the deck and returns it
{
  CardTypes* card = s[deckSize-1];  //Assign the pointer to card.
  delete s[deckSize-1];//DELETE THE INSTANCE (The memory that 

  return card; //The card points to a memory previously released.
}

Here is the moment that you are trying to use an unallocated:

CardTypes* card = d1.draw();//Get the pointer to s[deckSize-1]
cout<<"Card:"<<card->getCard() <<"\n";//crashes here

UPDATE:

Answer your comment, You can do this:

1.- Get the referencer to instance: CardTypes* card = s[deckSize-1];.

2.- Set the slot of your array in NULL and decrease the index:

s[deckSize-1]=NULL;
deckSize--;.

3.- Return de reference saved in card to upper level: return card;.

4.- Use the reference returned as you need it:

CardTypes* card = d1.draw();
cout<<"Card:"<<card->getCard() <<"\n";.

5.- Finally, delete de instance once you have finished to use it, for example just after the invocation of getCard():

cout<<"Card:"<<card->getCard() <<"\n";
delete card;

It's important to say that you decide where and when reserve, use and release the memory; just keep in mind to do it in an organized way and apply best practices, like these:

https://www.codeproject.com/Articles/13853/Secure-Coding-Best-Practices-for-Memory-Allocation http://www.embeddedstar.com/technicalpapers/pdf/Memory-Management.pdf

Answer 2

You are struggling with pointer ownership. You understand that in C++ one must delete a pointer when it is no longer needed, but in Deck::draw you delete a pointer when it is still needed.

CardTypes* Deck::draw()
{
    CardTypes* card = s[deckSize]; // s and card point to same allocation
    delete s[deckSize]; // boom! card points to garbage.
    s[deckSize] = NULL;
    deckSize--;
    return card;
}

You can use raw pointers, but you need to do it with a lot of coding maturity and deliberation.

Or you can say Smurf it and protect yourself from accidents like this with smart pointers. What is a smart pointer and when should I use one?

std::unique_ptr bundles ownership of a pointer. Only one std::unique_ptr is allowed at a time. You can't copy it. It as to be moved everywhere, transferring ownership from one holder to the next. But in the end there can be only one. You have to go out of your way to be stupid with a unique_ptr. Making five unique_ptrs and pointing them all at the same pointer, yeah you can do that. Put a self-destructing Automatic variable in a unique_ptr, yeah you can do that (and sometimes you do, but with a custom deleter that does nothing).

unique_ptr is the owner of the pointer. Whoever has the unique_ptr is owner by proxy because as soon as they get rid of the unique_ptr, the pointer goes with it.

Let's take a look at what we can do with a std::unique_ptr<CardTypes> to corral these wild pointers. If s is an array of unique_ptrs, std::unique_ptr<CardTypes> s[MAX_DECK_SIZE];, draw becomes

std::unique_ptr<CardTypes> Deck::draw()
{
    std::unique_ptr<CardTypes> card = std::move(s[deckSize]);
    // delete s[deckSize]; don't. Card now owns the card
    // s[deckSize] = NULL; handled by moving ownership
    deckSize--;
    return card;
}

This can be simplified to

std::unique_ptr<CardTypes> Deck::draw()
{
    return std::move(s[deckSize--]);
}

decksize-- is a post decrement so it happens after and the rest of the work is managed by the unique_ptr when it is moved out of s;

Sadly this means

s[i] = p[j];

ain't so easy anymore. You need

s[i] = std::move(p[j]);

But only if p no longer needs its jth element, because s[i] owns it now, baby.

Too little information has been provided in the question to wrangle this properly, but...

It's very possible that you could keep p full of unique_ptrs and load s with raw pointers whose lifespan is governed by p and pass s's pointers around naked and free without ever deleteing them because you know p has your back. So long as you keep p around longer than s and whoever s gives pointers to. It all comes back to ownership and in this case p owns all the Cards.

That turns

s[i] = std::move(p[j]);

into

s[i] = p[j].get();

and draw into

CardTypes * Deck::draw()
{
    return s[deckSize--];
}

and makes life really, really easy.