CODEWITHSUNDEEP
c++pointersswap
Temunish
Temunish

Reputation: 55

Swapping algorithm giving everything the same value

I have been given a task for my course to write a program which creates 52 card objects which each card has a value and a suit. After we create an array of the objects we have to switch the position of two cards at random. The problem is when I go to swap my cards that they both turn out to be the same. Here's the code that causes the problem:

void SwapCards(Card* cardOne, Card* cardTwo) {
    //swap cards here
    Card *temp = cardOne;
    *cardOne = *cardTwo;
    *cardTwo = *temp;
    //once it gets here all three variables end up with the same object?
}

Now here's the for loop that calls this function:

for (int i = 0; i < 104; i++) { 
    //get two random values and send them to SwapCards to switch both objects
    int c_one = RandomRange(0, 51);
    int c_two = RandomRange(0, 51);
    SwapCards(deck[c_one], deck[c_two]);
}

Any help with this will be greatly appreciated. I have spent a lot of time trying to figure it out but it just confuses me greatly.

Upvotes: 2

Views: 162

Answers (4)

πάντα ῥεῖ
πάντα ῥεῖ

Reputation: 1

"The problem is when I go to swap my cards that they both turn out to be the same."

You are loosing the current value of cardOne here:

*cardOne = *cardTwo;

Since temp still points to the same address as cardOne, cardOne's original value was not saved with it.

Change your code as follows, to save cardOne's value:

Card temp = *cardOne;
*cardOne = *cardTwo;
*cardTwo = temp;

The even better (and probably clearer) solution would be to use references instead of pointers:

void SwapCards(Card& cardOne, Card& cardTwo) {
    //swap cards here
    Card temp = cardOne;
    cardOne = cardTwo;
    cardTwo = temp;
}

And as a side note: This is what std::swap() would already do, no need to roll your own implementation here.

Upvotes: 5

Alnitak
Alnitak

Reputation: 339796

The standard way to write a "swap" function in C++ is to pass references instead of pointers:

void swap(Card &a, Card &b)
{
    Card tmp(a);  // or Card tmp = a;
    a = b;
    b = tmp;
}

There's no need to call the function swapCard because parameter matching means it can only ever be invoked with two Card arguments.

This can then be made almost completely generic using templates:

template <class T> void swap ( T& a, T& b )
{
    T tmp(a);  // construct a copy of a
    a = b;     // overwrite 'a'
    b = c;     // replace 'b' with the copy
}

This is such a common paradigm that it became standard - std::swap.

If the type T is compatible it "just works", otherwise you can implement a "specialization" of the template for a specific class. It's sometimes desirable to specialize for efficient reasons (e.g. you can swap member pointers instead of copying their contents).

Upvotes: 2

GreatAndPowerfulOz
GreatAndPowerfulOz

Reputation: 1775

Remove the * from temp in Card *temp = cardOne;. Instead use

Card temp = *cardOne;

then 

*cardTwo = temp;

Upvotes: 3

molbdnilo
molbdnilo

Reputation: 66371

When you enter the function, cardOne points to one card and cardTwo points to another card.

 Card *temp = cardOne;

temp now points to the card that cardOnepoints to.

 *cardOne = *cardTwo;

The card that cardOne points to now has the same value as the card that cardTwo points to. 

 *cardTwo = *temp;

The card that cardTwo points to now has the same value as the card that temp points to, which is the same card that cardOne points to – the card that you just gave the same value as the card cardTwo points to.
In other words, this assigns to *cardTwo the same value as *cardTwo already has.

For swapping, you want to save the value of *cardTwo, not its address.

Card temp = *cardOne;
*cardOne = *cardTwo;
*cardTwo = temp;

Upvotes: 0

Related Questions