CodeGuru Home VC++ / MFC / C++ .NET / C# Visual Basic VB Forums Developer.com
Page 1 of 2 12 LastLast
Results 1 to 15 of 26
  1. #1
    Join Date
    Oct 2009
    Posts
    19

    Talking 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.
    Attached Files Attached Files
    Last edited by exiledgolem; November 24th, 2009 at 12:28 PM. Reason: added source...

  2. #2
    GCDEF is offline Elite Member Power Poster
    Join Date
    Nov 2003
    Location
    Florida
    Posts
    12,635

    Re: My new GoFish thread.

    ++pplayer.iHand

    You want to increment the iterator, not the hand.

  3. #3
    Join Date
    Apr 2008
    Posts
    725

    Re: My new GoFish thread.

    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

  4. #4
    Join Date
    Jul 2005
    Location
    Netherlands
    Posts
    2,042

    Re: My new GoFish thread.

    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.
    Cheers, D Drmmr

    Please put [code][/code] tags around your code to preserve indentation and make it more readable.

    As long as man ascribes to himself what is merely a posibility, he will not work for the attainment of it. - P. D. Ouspensky

  5. #5
    Join Date
    Apr 2008
    Posts
    725

    Re: My new GoFish thread.

    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 :-/
    Last edited by Amleto; November 24th, 2009 at 01:11 PM.

  6. #6
    Join Date
    Oct 2009
    Posts
    19

    Re: My new GoFish thread.

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

  7. #7
    Join Date
    Apr 2008
    Posts
    725

    Re: My new GoFish thread.

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

  8. #8
    Join Date
    Jun 2009
    Location
    France
    Posts
    2,513

    Re: My new GoFish thread.

    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!

  9. #9
    Join Date
    Oct 2009
    Posts
    19

    Re: My new GoFish thread.

    Quote Originally Posted by monarch_dodra View Post
    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*...
    Last edited by exiledgolem; November 24th, 2009 at 08:52 PM.

  10. #10
    Join Date
    Jun 2009
    Location
    France
    Posts
    2,513

    Re: My new GoFish thread.

    Quote Originally Posted by exiledgolem View Post
    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!

  11. #11
    Join Date
    Oct 2009
    Posts
    19

    Re: My new GoFish thread.

    Quote Originally Posted by monarch_dodra View Post
    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

  12. #12
    Join Date
    Nov 2008
    Location
    England
    Posts
    748

    Re: My new GoFish thread.

    @ 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.
    Get Microsoft Visual C++ Express here or CodeBlocks here.
    Get STLFilt here to radically improve error messages when using the STL.
    Get these two can't live without C++ libraries, BOOST here and Loki here.
    Check your code with the Comeau Compiler and FlexeLint for standards compliance and some subtle errors.
    Always use [code] code tags [/code] to make code legible and preserve indentation.
    Do not ask for help writing destructive software such as viruses, gamehacks, keyloggers and the suchlike.

  13. #13
    Join Date
    Oct 2009
    Posts
    19

    Re: My new GoFish thread.

    Quote Originally Posted by Russco View Post
    @ 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...

  14. #14
    Join Date
    Jun 2009
    Location
    France
    Posts
    2,513

    Re: My new GoFish thread.

    Quote Originally Posted by Russco View Post
    @ 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.

  15. #15
    Join Date
    Jan 2006
    Location
    Singapore
    Posts
    6,765

    Re: My new GoFish thread.

    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:eque, 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:eque iterators can be invalidated on any insertion or deletion from the std:eque.
    C + C++ Compiler: MinGW port of GCC
    Build + Version Control System: SCons + Bazaar

    Look up a C/C++ Reference and learn How To Ask Questions The Smart Way
    Kindly rate my posts if you found them useful

Page 1 of 2 12 LastLast

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  





Click Here to Expand Forum to Full Width

Featured