My new GoFish thread.

Well... long story short I had another thread about a month ago ? maybe a little later.. I went through the design process like.. 10 times and now I think I finally have my final design pretty pat..

I'm only having one simple issue.. I have a class Card that holds and int Face(1 ace, 2 two, 13 king) and int Suit (1 heart, 4 spade). Player hands are a list of card objects, same goes for the deck..

Code:

---

list <Card> Contents..
list <Card> Hand

---

I want to pushback a copy of a card from the deck, and put it into my hand..

Code:

---

list <Card>::iterator i = Deck.Contents.begin();
**** iterate through list *****
this->Hand.pushback(*i);

---

It seems to work for one iteration but .. only one...
Does anyone have any ideas? I've looked everywhere...
I tried allocating a new card new Card (suit, face) and trying to copy it over insert..
but I'm a little lost...

edited to add source.

++pplayer.iHand

You want to increment the iterator, not the hand.

I get this:

Code:

---

error C2664: 'std::list<_Ty>::push_front' : cannot convert parameter 1 from 'std::list<_Ty>::_Iterator<_Secure_validation>' to 'const Card &'

---

Code:

---

void Player::PickupCards(const int NumberOfCards)  // pickup a certain amount of cards from our global deck
{
    EndGame();              // check to see if the deck vector is empty, ifso endgame, display winner
       

    int Count = 0;                  // counter to end loop when == numberofcards we want to take
       
        for (list <Card>::iterator i = Deck.Contents.begin(); i != Deck.Contents.end(); ++i) // go backwards....
    {
        Hand.push_front(i); // here

---

You have this code:

Code:

---

void Player::PickupCards(const int NumberOfCards)  // pickup a certain amount of cards from our global deck
{
        //...
        for (list <Card>::iterator i = Deck.Contents.begin(); i != Deck.Contents.end(); ++i) // go backwards....
    {
        Hand.push_front(i);      // take the last value from the deck, and put it into that players hand
        Deck.Contents.pop_front();                      // remove the last value of the deck, shortening it, so value was removed

        //...
    }
}

---

When you do a pop_front you invalidate the iterator i. Therefore, you have undefined behaviour.

Code:

---

for (list <Card>::iterator i = Deck.Contents.begin(); i != Deck.Contents.end(); ++i) // go backwards....
    {
        Hand.push_front(*i);      // take the last value from the deck, and put it into that players hand
        Deck.Contents.pop_front();                      // remove the last value of the deck, shortening it, so value was removed

        EndGame();                          // since we removed a value, make sure the deck still has values in it
                                                    // so we can draw cards

        ++Count;                                // ifso, increase count

        if (Count == NumberOfCards)                // if count == numberofcards
        break;                                      // stop picking cards up
    }

---

you invalidate the iterator!

edit: posted before refreshing :-/

thank you all , I found out it was such a simple fix

before code..

Code:

---

void Player::PickupCards(const int NumberOfCards)  // pickup a certain amount of cards from our global deck
{
    EndGame();              // check to see if the deck vector is empty, ifso endgame, display winner
       

    int Count = 0;                  // counter to end loop when == numberofcards we want to take
       
        for (list <Card>::iterator i = Deck.Contents.begin(); i != Deck.Contents.end(); ++i) // go backwards....
    {
        Hand.push_front(*i);      // take the last value from the deck, and put it into that players hand
        Deck.Contents.pop_front();                      // remove the last value of the deck, shortening it, so value was removed

        EndGame();                          // since we removed a value, make sure the deck still has values in it
                                                    // so we can draw cards

        ++Count;                                // ifso, increase count

        if (Count == NumberOfCards)                // if count == numberofcards
        break;                                      // stop picking cards up
    }
}

---

after code

Code:

---

void Player::PickupCards(const int NumberOfCards)  // pickup a certain amount of cards from our global deck
{
    EndGame();              // check to see if the deck vector is empty, ifso endgame, display winner
       

    int Count = 0;                  // counter to end loop when == numberofcards we want to take
       
        for (list <Card>::iterator i = Deck.Contents.begin(); i != Deck.Contents.end(); ) // go backwards....
    {
        Hand.push_front(*i);      // take the last value from the deck, and put it into that players hand
                ++i;
        Deck.Contents.pop_front();                      // remove the last value of the deck, shortening it, so value was removed

        EndGame();                          // since we removed a value, make sure the deck still has values in it
                                                    // so we can draw cards

        ++Count;                                // ifso, increase count

        if (Count == NumberOfCards)                // if count == numberofcards
        break;                                      // stop picking cards up
    }
}

---

I just had to move the ++i after I copied the value...

or rather, before you pop the front (and therefore 'i')

Hi! I remeber helping you when way back. Good to see you kept working on your program.

Looks like you found your problem, which is good, but I'll still tell you what I think about some parts of your code:

StandardDeck::Shuffle. You are aware that algorithm::random_shuffle does the same thing? You are re-writing the wheel on that one. Not to mention you also re-invented the swap function. Take a look here. http://www.cplusplus.com/reference/a...andom_shuffle/ it's not very complicated either.


Your Card::toString. I remember with Paul telling you to use an array, like this:

Code:

---

std::string gValues[] = {"Ace", "Two", ... , "King"};
...
cout << gValues[aCard.Value];

---

Something like that. I'm not going to re-write the entire code. But using this kind of solution is generally more maintainable, and has better performance.

Avoid doing this:

Code:

---

class MyClass{
...
}gloableName1, globaleName2

---

Where there is nothing wrong with doing it that way, has some problems:
- 1. People just plain aren't used to this kind of construct.
- 2. It's not as clear as having an area right before main labeled "//Global Variables"
- 3. You won't be able to move your classes away into their own .h

There. The code looks good!

Quote:

---

Originally Posted by monarch_dodra

Hi! I remeber helping you when way back. Good to see you kept working on your program.

Looks like you found your problem, which is good, but I'll still tell you what I think about some parts of your code:

StandardDeck::Shuffle. You are aware that algorithm::random_shuffle does the same thing? You are re-writing the wheel on that one. Not to mention you also re-invented the swap function. Take a look here. http://www.cplusplus.com/reference/a...andom_shuffle/ it's not very complicated either.


Your Card::toString. I remember with Paul telling you to use an array, like this:

Code:

---

std::string gValues[] = {"Ace", "Two", ... , "King"};
...
cout << gValues[aCard.Value];

---

Something like that. I'm not going to re-write the entire code. But using this kind of solution is generally more maintainable, and has better performance.

Avoid doing this:

Code:

---

class MyClass{
...
}gloableName1, globaleName2

---

Where there is nothing wrong with doing it that way, has some problems:
- 1. People just plain aren't used to this kind of construct.
- 2. It's not as clear as having an area right before main labeled "//Global Variables"
- 3. You won't be able to move your classes away into their own .h

There. The code looks good!

---

Thanks for the comments.

As for the random_shuffle routine... I use lists, and it's not available for lists? Unless I'm just missing it when I type in Deck.Contents....it's just not there.. You are right about swap though.. I just thought I would exercise that little ol'brain of mine... considering random_shuffle wasn't there.

Long story short as for that toString function... I tried going about doing it paul's way, and for some reason as for which I do not remember, it just seemed like it didn't "fit" well with me. It might have been a design flaw on my part, which I believe is fixed now, but I don't really care about the speed of it. I'm more concerned as to just get this thing out and working to finally say I've finished something on my own.

I actually just went to the bookstore recently and bought a book on general algorithms and their everyday use in programming. Great book. I know it'll help me down the line on any speed inprovements I might want to take a look at if I get into anything serious.

As for the global objects, I was doubting myself too when it came down to my methods. I'll take your advice mainly because you are able to distinguish it easier.

Thanks again everyone for the great advice!

I still need to learn as to why my iterators are becoming invalidated in certain situations... I still have a problem with my looking for card from another player function

Code:

---

void Player::CheckFor (const int face, Player& pplayer) // need set value 1 for twos.. 13 for aces, 4 for fives
{
    list <Card>::iterator i;

    for (i = pplayer.Hand.begin(); i != pplayer.Hand.end(); ++i)            // for that players whole hand
    {
        if ((*i).Face == face)
        {       
          Hand.push_front(*i);       
          i = pplayer.Hand.erase(i);                               
        }
    }
}

---

.. I'm using the exact same methods as the draw card function but I run into problems eghh... *sigh*...

Quote:

---

Originally Posted by exiledgolem

Thanks for the comments.

As for the random_shuffle routine... I use lists, and it's not available for lists? Unless I'm just missing it when I type in Deck.Contents....it's just not there.. You are right about swap though.. I just thought I would exercise that little ol'brain of mine... considering random_shuffle wasn't there.

Long story short as for that toString function... I tried going about doing it paul's way, and for some reason as for which I do not remember, it just seemed like it didn't "fit" well with me. It might have been a design flaw on my part, which I believe is fixed now, but I don't really care about the speed of it. I'm more concerned as to just get this thing out and working to finally say I've finished something on my own.

I actually just went to the bookstore recently and bought a book on general algorithms and their everyday use in programming. Great book. I know it'll help me down the line on any speed inprovements I might want to take a look at if I get into anything serious.

As for the global objects, I was doubting myself too when it came down to my methods. I'll take your advice mainly because you are able to distinguish it easier.

Thanks again everyone for the great advice!

I still need to learn as to why my iterators are becoming invalidated in certain situations... I still have a problem with my looking for card from another player function

Code:

---

int CheckFor (const int face, Player& pplayer)

---

.. I'm using the exact same methods as the draw card function but I run into problems eghh... *sigh*...

---

For iterator invalidation, but this is generally true when you use the stl:

You MUST know every container, and their functions by heart, and which invalidate what. It is easier if you understand how the container works. If you don't take the time to learn this, you will run into trouble.

As for random shuffle. It did not appear because it is a generic algorithm, like all algorithms in the stl. You are just supposed to call it between begin and end:

Code:

---

std::list<T> myList;
...
std::random_suffle(myList.begin(), myList.end());

---

It is true that some algorithms were re-written specifically for lists (sort comes to mind). But as a general rule, the algorithms are not member functions of your containers. This is what allows you to call them on anything, even plain old arrays!

Quote:

---

Originally Posted by monarch_dodra

For iterator invalidation, but this is generally true when you use the stl:

You MUST know every container, and their functions by heart, and which invalidate what. It is easier if you understand how the container works. If you don't take the time to learn this, you will run into trouble.

As for random shuffle. It did not appear because it is a generic algorithm, like all algorithms in the stl. You are just supposed to call it between begin and end:

Code:

---

std::list<T> myList;
...
std::random_suffle(myList.begin(), myList.end());

---

It is true that some algorithms were re-written specifically for lists (sort comes to mind). But as a general rule, the algorithms are not member functions of your containers. This is what allows you to call them on anything, even plain old arrays!

---

UGH, I FORGOT... I feel so dumb. BTW, I JUST figured out my problem... I forgot the erase function.. when you say i = ***.erase(blah), it automatically moves the iterator to the next position... so I just added an else statement and it works perfectly now... ugh >_> THANK YOU SOOO MUCH <3

@ MD

random_shuffle requires random access iterators. list does not support random access iterators, list iterators are bidirectional. To use random_shuffle the OP would need swop his lists for vectors or deques.

Quote:

---

Originally Posted by Russco

@ MD

random_shuffle requires random access iterators. list does not support random access iterators, list iterators are bidirectional. To use random_shuffle the OP would need swop his lists for vectors or deques.

---

yeah I thought I was going crazy there for a second...
I actually deleted my modified shuffle routine ... and tried random_shuffle again.. to no avail ...
it was like.. NOOOO OMG >__________< *ctrl z ctrl z ctrl z*

thanks... again...

Quote:

---

Originally Posted by Russco

@ MD

random_shuffle requires random access iterators. list does not support random access iterators, list iterators are bidirectional. To use random_shuffle the OP would need swap his lists for vectors or deques.

---

Strange... Most of the algorithms in the std work with simple forward iterators. They work better with random access, but I thought it would still work with forward iterators.

And looking at the algorithm, I see no reason why it wouldn't work. But it doesn't matter, the standard is the standard, and random_shuffle doesn't work. I stand corrected :)

@OP, I still recommend you use the algorithm inside the link I gave you. Not only is it much lighter (only N iterations, whereas you do magic number 10000), but it gives you a true random shuffle, i.e., it gives you a uniform distribution on the N! possible permutations.

Quote:

---

Originally Posted by monarch_dodra

Strange... Most of the algorithms in the std work with simple forward iterators. They work better with random access, but I thought it would still work with forward iterators.

And looking at the algorithm, I see no reason why it wouldn't work. But it doesn't matter, the standard is the standard, and random_shuffle doesn't work. I stand corrected

---

It might be an oversight in the standard, but it might also be because we're talking about linear time complexity degrading to quadratic time complexity, if my analysis is correct. Consequently, it might be "dangerous" to allow it to work with mere forward iterators, since it would likely be considerably better to shuffle an auxiliary container with random access.

Speaking of containers with random access... exiledgolem, I am guessing that you chose std::list over std::vector because you want to pop/push front/back, and generally did not need the random access. However, perhaps you should consider std::deque, which would allow you to pop/push front/back in constant time, and also provides random access. Unfortunately, it is not suitable if you need to store, in other objects, iterators to the elements of that container, since std::deque iterators can be invalidated on any insertion or deletion from the std::deque.

Quote:

---

Originally Posted by laserlight

It might be an oversight in the standard, but it might also be because we're talking about linear time complexity degrading to quadratic time complexity, if my analysis is correct. Consequently, it might be "dangerous" to allow it to work with mere forward iterators, since it would likely be considerably better to shuffle an auxiliary container with random access.

Speaking of containers with random access... exiledgolem, I am guessing that you chose std::list over std::vector because you want to pop/push front/back, and generally did not need the random access. However, perhaps you should consider std::deque, which would allow you to pop/push front/back in constant time, and also provides random access. Unfortunately, it is not suitable if you need to store, in other objects, iterators to the elements of that container, since std::deque iterators can be invalidated on any insertion or deletion from the std::deque.

---

Could be, could be. I always thought the philosophy behind the std was "we give you the best tools we can, but it is your responsability to use them right".

To get back to OP, though, why do you need to popFront? Your cardStack (I'm avoiding the word deque) can only be read from one "tip" right? Why don't you just use back() and pop_back()? That would solve a bunch of your problems. Also, you will be able to use a vector, and random_suffle.

Quote:

---

Originally Posted by monarch_dodra

Could be, could be. I always thought the philosophy behind the std was "we give you the best tools we can, but it is your responsability to use them right".

To get back to OP, though, why do you need to popFront? Your cardStack (I'm avoiding the word deque) can only be read from one "tip" right? Why don't you just use back() and pop_back()? That would solve a bunch of your problems. Also, you will be able to use a vector, and random_suffle.

---

It was originally done using popback and I only found out I made more problems by doing with popfront etc, so I changed it back last night. You're right about the vector, but I could only use a vector for the deck, not the hand consider the check for cards function from the hand removes a card from ANY place thus invalidating the iterator.. I already went through that ;), so I'll just keep a list for both.

Well everyone.. I'm FINALLY FINALLY FINALLLyyyyy finished with the whole program.

There are a lot of improvements I'm going to make to it.
But the program is finally functional and error prone.

A couple quick things though...

You may choose ANY card to ask for, even if you don't have it in your hand...
I plan to change this asap, to where you can't, because I believe that is the "norm?"

I only support 2 players and 1 deck right now..
I plan to eventually do it for 8 players / 4 decks..

Gofish currently doesn't show the sets of cards that have been removed
from the game. Shouldn't be too hard to add in.

I also want to add which card was drawn from the Deck..
string style...

add some timers for the highscores.. so the time in minutes / seconds + the number of sets taken... output it to a file... well of course encrypt the information first...

add a name to the player class. Add some options for decks / players / maybe a couple
rules. Add some credits for all who have helped :))) if you'd like.

My longterm goal was actually to change this whole concept over to a basic console
input / output with graphics with the SDL library (which I FINALLY found out how to link it using my compiler Visual Studio 2008 prof).

I am still a little iffy on the program control in the main loop...

but thanks again all! Any questions, comments, or concerns... pleeeaseeee post them.

You still need to realize when you're repeating code, or when the code has an obvious pattern to it. This code could be half the size it is now if you do not repeat code blocks that only differ by a constant or a variable.

The sign of a good programmer is one that recognizes repeated code, and rewrites it so that the code does not repeat. For example:

Code:

---

void Card::toString()               
{
  //...
  static const char* FaceOuput[] = {"Ace", "Two", "Three", "Four" /*etc */};
  if ( Face >=1 && Face <= 12 )
      cout << FaceOutput[Face - 1];
  else
        cout << "ERROR";
}

---

That gets rid of over half of your toString() function.

Regards,

Paul McKenzie

Quote:

---

Originally Posted by Paul McKenzie

You still need to realize when you're repeating code, or when the code has an obvious pattern to it. This code could be half the size it is now if you do not repeat code blocks that only differ by a constant or a variable.

The sign of a good programmer is one that recognizes repeated code, and rewrites it so that the code does not repeat. For example:

Code:

---

void Card::toString()               
{
  //...
  static const char* FaceOuput[] = {"Ace", "Two", "Three", "Four" /*etc */};
  if ( Face >=1 && Face <= 12 )
      cout << FaceOutput[Face - 1];
  else
        cout << "ERROR";
}

---

That gets rid of over half of your toString() function.

Regards,

Paul McKenzie

---

Yeah, I implemented it. It helped a lot. Thanks for the advice. I've come into another problem though. Streams.. gosh those streams.. just getting correct input.. I've finally got it down.. but I want to limit the cards the player can choose to just those in their own hand.. so if they only have 5's, 7's, and 8's they can only ask for those cards..

Code:

---

do
{
    // a lot of cout stuff
    cin >> myChoice // ask for which card they want to grab from the other player
   
    if (!cin)
    {
          cin.clear();
          cin.ignore(numeric_limits<streamsize>::max(), '\n');
    }

} while (myChoice > 13 || myChoice < 1 || (Me.badInput()==true));

---

Code:

---

bool Player::badInput()
{
        list <int> temp;

        list <Card>::iterator i;
        list <int>::iterator b;

        i = this->Hand.begin();

        for (i; i != Hand.end(); ++i)
        {
                temp.push_back(i->Face); 
        }

        b = temp.begin();

        for (b; b != temp.end(); )
        {
                if (!((*b) ==  myChoice))
                {
                        cout << "\n\n\tYou can't choose a card you don't have.\n\t";
                        system ("pause");
                        system ("cls");
                        return true;
                }

                else
                ++b;
        }

        return false;
}

---

I believe my algorithm is correct to see if they are asking for a card that isn't in their hand.
The function returns true, but yet, in the program... it keeps sticking saying that they are asking for a card they don't have,
100 percent of the time, no matter what they input.

I don't know what I'm doing wrong. I've been at this one for 3 + hours now using google. The function asks for the input.
I've tried it a couple different ways, but nothing right yet.

Quote:

---

Originally Posted by exiledgolem

Yeah, I implemented it. It helped a lot. Thanks for the advice. I've come into another problem though. Streams.. gosh those streams.. just getting correct input..

---

Worry about the algorithm and structure of your program first before you talk about the user interface.

Anyway, input should be separated from the actual program engine. If you decide to change this to a GUI program, and if you tightly integrate the input with the GoFish logic, you will have a hard time separating or breaking down your program.

Quote:

---

I've finally got it down.. but I want to limit the cards the player can choose to just those in their own hand.. so if they only have 5's, 7's, and 8's they can only ask for those cards..

---

Write a function that returns the valid cards a user can ask for, given what they have. That is the only thing you should do, and again, not try to tightly couple I/O with this. You'll just get yourself in a mess if you mix up I/O with the GoFish logic.

Regards,

Paul McKenzie

Quote:

---

Originally Posted by Paul McKenzie

Worry about the algorithm and structure of your program first before you talk about the user interface.

Anyway, input should be separated from the actual program engine. If you decide to change this to a GUI program, and if you tightly integrate the input with the GoFish logic, you will have a hard time separating or breaking down your program.
Write a function that returns the valid cards a user can ask for, given what they have. That is the only thing you should do, and again, not try to tightly couple I/O with this. You'll just get yourself in a mess if you mix up I/O with the GoFish logic.

Regards,

Paul McKenzie

---

Thank you for the great advice. I was thinking this, and it's not REALLY tightly integrated with it. I actually plan to move all the classes to header's and make just an input header to seperate everything (including removing cout in the main logic classes' functions). I was only doing it as a short-term thing. I wanted to finish a project for once, but I definately will take your advice and seperate everything pretty soon. It's still a mess for me. I thought I couldn't return more then one object for a value though? I'll look into that one.. because that was my first though... maybe return a vector / list... but returning mutiple card objects would be EVEN better.

Quote:

---

Originally Posted by exiledgolem

because that was my first though... maybe return a vector / list... but returning mutiple card objects would be EVEN better.

---

You would return some sort of container (vector, list, etc.) of the cards that can be chosen. This function shouldn't get involved in any I/O at all. The function shouldn't know whether you are writing a simple console program, GUI program, etc... It shouldn't even know what part of the game is being executed. All it knows that it has a list of cards, and given this list, return another list that shows what cards can be chosen.

The caller to this function can do whatever it wants with the returned list of cards.

Regards,

Paul McKenzie

Actually Paul I was able to fix my problem without the use of returning a container (thankfully).
I reread my logic a million times and I guess I was just going about it wrong. I finally came up with this idea.

Code:

---

bool Player::badInput(const int input)                // returns true if the value set as a parameter do not match any card in their hand
{                                                                                        // example, Me.badInput(3) .. 3 is a three of any kind.. if I'm asking for a 3 and it's not in my hand
                                                                                        // the function returns true, and false if my choice is equal to ANY one of my cards in my hand.
       
        int count = 0;                                                        // used to count if ANY card is equal to my input

        list <Card>::iterator i;
        i = Hand.begin();

        for (i; i != Hand.end(); ++i)                        // go through the whole hand.
        {
                if (input == (*i).Face)                                // if the input is equal to the face value of that current card
                        ++count;                                                // increase the count
        }

        if (count > 0)                                                        // if we got a match with one of cards in our hand with our input
                return false;                                                // we got good input
        else
                return true;                                                // if no cards matched... it's bad input, returns true

}

---

I started to work on the highscores and the encryption now.. and what a PAIN..
hahah <3 thanks again though everyone.

Just a simple ideal would be to change this

Code:

---

   
if (count > 0)     
    return false;
else
    return true;

---

to

Code:

---


    return count == 0;

---

Quote:

---

Originally Posted by exiledgolem

Actually Paul I was able to fix my problem without the use of returning a container (thankfully).

---

Here is the same code:

Code:

---

bool Player::badInput(int input)
{
  for (list <Card>::iterator i = Hand.begin(); i != Hand.end(); ++i)               
  {
        if (input == (*i).Face)
            return false;
  }
  return true;
}

---

OR

Code:

---

#include <algorithm>
//...
struct InputChecker
{
  InputChecker(int input) : m_input(input) { }

  bool operator() (const Card& card) const
  { return card.Face == m_input; }

  int m_input;
};

bool Player::badInput(int input)
{
  InputChecker(input) IC;
  return std::find_if( Hand.begin(), Hand.end(), IC) ==  Hand.end());
}

---

Regards,

Paul McKenzie