My GoFish Program

[exiledgolem]

Seems like there is a bug in it, and I can't really find out where. I'm not even sure if my algorithms are the best way of going about things but any help would be awesome.
It seems like my bug might have to deal with my checking for points algorithm... when I ask for input of a set that has already been made.. it's a longshot for a guess but it's a start..

[Paul McKenzie]

Seems like there is a bug in it, and I can't really find out where.

Did you use your compiler suite's debugger?

However, a lot of your code could be cleaned up. Your "CheckForPoints" function could be much smaller:

void CPlayer::CheckForPoints(void)    // remove cards from hand and give correct player points for each set
{
   struct PointsToPick
   {
        int point1;
        int point2;
        int point3;
        int point4;
   }

   const PointsToPick AllPoints[] = { {1,14,27,40}, {2,15,28,41}, {3,16,29,42}, {4,17,30, 43} /* etc ... */ };

   const int numPointsToPick = sizeof(AllPoints) / sizeof(AllPoints[0]);

   for (int i = 0; i < numPointsToPick; ++i )
   {
	if ((PickedUp[AllPoints[i].point1] == AllPoints[i].point1) && 
            (PickedUp[AllPoints[i].point2] == AllPoints[i].point2) && 
            (PickedUp[AllPoints[i].point3] == AllPoints[i].point3) && 
            (PickedUp[AllPoints[i].point4] == AllPoints[i].point4) )
	{    
		Points += 1;   
		PickedUp[AllPoints[i].point1] = -1;  // set each card to -1 saying set was made
		PickedUp[AllPoints[i].point2] = -1;
		PickedUp[AllPoints[i].point3] = -1;
		PickedUp[AllPoints[i].point4] = -1;
		
		for (int curHand = 0; curHand < 52; ++curHand) // for whole hand
		{
			if ((Hand[curHand] == AllPoints[i].point1) || 
                            (Hand[curHand] == AllPoints[i].point2) || 
                            (Hand[curHand] == AllPoints[i].point3) || 
                            (Hand[curHand] == AllPoints[i].point4))
			{  
				Hand[curHand] = 0;  
			}
		}
          }
    }	
	
    if (Taken == 52)   // if we took all the cards end the game
    {
       system ("cls");
       cout << "\t\tGame over!\n\n\n\t\t";   // endgame function
            
       if (ME.Points > OPPONENT.Points)
       {
          cout << "YOU WIN!";
       }
            
       else 
       {
          cout << "OPPONENT WINS!";
       }
            
       system ("pause");
       exit(1);
    }
}

Also, your constructor is a one line initialization, instead of all of that code you have now.

CPlayer::CPlayer () : Hand(52,0), Turn(0), Points(0), PickedUp(53, 0)
{ 
}

The above is equiavlent.

More clean up of the code. All of this can be simplified:

cout << "\n\n\n\tSets Taken: \n\t";
  for (int i = 0; i < Count; i++)     // used count here
  {
      if (SetsTaken[i] == 1)           // if just one value was put into the vector// the rest were made a set
      {
         cout << "TWOS ";              // display set that you have made
      }
      
      if (SetsTaken[i] == 2)
      {
         cout << "THREES ";
      }
      
      if (SetsTaken[i] == 3)
      {
         cout << "FOURS ";
      }
      
      if (SetsTaken[i] == 4)
      {
         cout << "FIVES ";
      }
      
      if (SetsTaken[i] == 5)
      {
         cout << "SIXES ";
      }
      
      if (SetsTaken[i] == 6)
      {
         cout << "SEVENS ";
      }
      
      if (SetsTaken[i] == 7)
      {
         cout << "EIGHTS ";
      }
      
      if (SetsTaken[i] == 8)
      {
         cout << "NINES ";
      }
      
      if (SetsTaken[i] == 9)
      {
         cout << "TENS ";
      }
      
      if (SetsTaken[i] == 10)
      {
         cout << "JACKS ";
      }
      
      if (SetsTaken[i] == 11)
      {
         cout << "QUEENS ";
      }
      
      if (SetsTaken[i] == 12)
      {
         cout << "KINGS ";
      }
      
      if (SetsTaken[i] == 13)
      {
         cout << "ACES ";
      }
  }
}

Do this instead:

cout << "\n\n\n\tSets Taken: \n\t";
const char *Output[] = { "", "TWOS ", "THREES ", "FOURS " /* etc. */ };
//..
  for (int i = 0; i < Count; i++)     // used count here
  {
      cout << Output[SetsTaken[i]];
  }

I don't like this.

void GoFish::EndGame(void)
{
     exit(1);  // simple exit
}

Classes should not be exiting the entire program like this. You should set a member stating that the game has exited, and just return. Let the caller decide what to do afterwards.

There are a lot of other things you could do to clean the code up. Above is only some of them.

Regards,

Paul McKenzie

[monarch_dodra]

Seems like there is a bug in it, and I can't really find out where. I'm not even sure if my algorithms are the best way of going about things but any help would be awesome.
It seems like my bug might have to deal with my checking for points algorithm... when I ask for input of a set that has already been made.. it's a longshot for a guess but it's a start..

Hi exiledgolem,

I looked at your code. Very nice, and well documented. I have to agree with Paul, some of those things in there could be heavily simplified. In particular, you have a heavy use of "magic numbers".

I would recommend you create a "card" struct, just for the heck of management, that can transform a card index into a value and a suit, like this:

struct card
{
    card(int intputIndex) : value(intputIndex%13), suit(intputIndex/13)
    {}
    card(int value, int suit) : value(value), suit(suit)
    {}

    const int value;
    const int suit;
} 

This way, you can understand your own code easier, by writing:

myCards[i]==card(9,3)

You know you are checking against the 9 of clubs.
You could make this even better by using an enum for your suits.
And with the single it constructor, you can still generate an entire deck by counting from 0 to 51 (although you could also be counting 4 times from 0 to twelve...)

Here is what I don't like about your program interface:
When the computer tells me that I have card "42" in my hand. What is that?
When the computer makes me write 4 when I want to check for a 5. Make the indexes match.

Here is what I don't like about your implementation:
You are using vectors, yet they always have a size of 52. In my opinion, you should be removing cards when they aren't there, rather than setting them to 0. This would also allow you to ditch the global "index" and "taken". Simply use pop_back for a new card, and check empty() to know if the game is over.
I know people say removing things from a vector is expensive, but not when it is from the back (as should be the case for the main deck). As for player hands, given the size, I don't really think you care.
I don't think CPlayer should be taking the one keeping track of who's turn it is. That is not its job.

Here is what is wrong about the program, simply by running it:
You never "go fish". You forgot to make the user draw a card if he fails to take one from the opponent. You never call CPlayer::Pickup.
Your opponent never plays. You never set your (or his) turn, except at construction time...

Other than that, not much. Please come back if yo are having more problems.

[exiledgolem]

thanks for all the wonderful suggestions, but my main problem wasn't with the simplifying of the code, even though I did ask for help
I was more curious as to why the program was crashing, I purposely removed the logic of switching between players because the program was crashing while I was just playing.

Wonderful suggestions though, I was only getting the rough draft first before I went back.. I'm a little confused on the whole struct idea, but I have a couple books and I'll take a deeper look at it when I get home I'm sure.

I have Visual Studio Prof 2008 on my normal computer, but I don't really know how to use the debugger that well, and it actually is not in service anymore, so I'm just using DevC++ bloodshed compiler...

If anyone could even manage to find out where that error might be, or if I should just rego about doing those suggestions and maybe it'll fix itself.. idk

thanks in advance though

[Paul McKenzie]

thanks for all the wonderful suggestions, but my main problem wasn't with the simplifying of the code, even though I did ask for help
I was more curious as to why the program was crashing, I purposely removed the logic of switching between players because the program was crashing while I was just playing.

You need to put the code back in, and tell us the exact steps/data/input needed to duplicate the problem.

I'm a little confused on the whole struct idea,

A struct is nothing more than a class with different default access specifiers. Other than that, there is no difference between a struct and a class, and your code already has classes in it.

What I did was just create an array of the struct, and just create the array with the data you have in your loop. The pattern in your code was obvious, and the solution I chose was just to "factor out" the code that gets done repeatedly, and just feed this code different data (i.e. each element of the struct array, for example).

I have Visual Studio Prof 2008 on my normal computer, but I don't really know how to use the debugger that well, and it actually is not in service anymore, so I'm just using DevC++ bloodshed compiler...

The debugger for Visual Studio is very easy -- all you had to do was press the "F10" key once the application was built, and then keep pressing F10 once the debugger highlighted the first executable line. From there, you should have been able to figure out what is going on, and then learn the other things such as setting breakpoints, creating watch variables, etc.

The Bloodshed IDE is no longer being updated, and it hasn't been for several years. I suggest you use CodeBlocks for your IDE instead of Bloodshed. You have a debugger in CodeBlocks (graphical gdb) that works on the same concepts as the Visual Studio debugger -- breakpoints, single stepping through the code, watches, etc.

Regards,

Paul McKenzie

[exiledgolem]

THanks again for your help, I didn't know the bloodshed IDE was out of date...
I'll come back if I have any questions, I'm going to try to rework most the algorithms
to include some of your suggestions, but instead of making a stuct of card

like stuct card
{
int suit;
int value;
}

why not do something like

int IsEqualTo (int value, int suit)
{
return (Suit * Value)
}

if (1-13 enumeration for the cards)(1-4 enumeration for the suits 1)

idk.. just a new idea... it'll definately be more readable.. if (Hand[i] == IsEqualTo(ACE, HEARTS))
I still wouldn't get the point for creating a struct like that..

I do love your idea though about the struct for the points..
definately save me some time.

thanks again though for the ideas... I'll come back a new version soon..

[monarch_dodra]

THanks again for your help, I didn't know the bloodshed IDE was out of date...
I'll come back if I have any questions, I'm going to try to rework most the algorithms
to include some of your suggestions, but instead of making a stuct of card

like stuct card
{
int suit;
int value;
}

why not do something like

int IsEqualTo (int value, int suit)
{
return (Suit * Value)
}

if (1-13 enumeration for the cards)(1-4 enumeration for the suits 1)

idk.. just a new idea... it'll definately be more readable.. if (Hand[i] == IsEqualTo(ACE, HEARTS))
I still wouldn't get the point for creating a struct like that..

I do love your idea though about the struct for the points..
definately save me some time.

thanks again though for the ideas... I'll come back a new version soon..

Why not? No reason, it's just 1 possible solution. Another advantage is that you can redefine:
ostream& operator<<(ostream&, const CCard&)
This way, when you write "cout << myCard << endl", it automatically generates "Four of Spades".
Also, IMO, it is more natural to write "myCard == CCard(5,SPADES)".

PS, the formula is:

int IsEqualTo (int value, int suit)
{
   return ((Suit-1)*13 + Value -1)
}

[exiledgolem]

thank you for your suggestion, I've made a lot of fixes to the program
and that would be awesome help, if you could provide an example of that operator that would be awesome... I'll probably end up figuring it out before you respond anyways but just in case

I'm sure within the next day or so I'll end up posting the revised program.
Thank you everybody for all your help.

[GNiewerth]

I recommend the use of arrays of card/suit name to look up card names:

#include <string>

const std::string SuitNames[] = 
{ 
   "Clubs", 
   "Spades", 
   "Hearts", 
   "Diamonds" 
};

const std::string FaceNames[] = 
{ 
   "Deuce", 
   "Three", 
   "Four", 
   "Five", 
   "Six", 
   "Seven", 
   "Eight", 
   "Nine", 
   "Ten", 
   "Jack", 
   "Queen", 
   "King", 
   "Ace" 
};

// I also recommend the use of enumerations for suit and face value
enum Suit
{
   Clubs = 0,
   Spades = 1,
   Hearts = 2,
   Diamonds = 3
};

enum Face
{
   Deuce = 0,
   Three = 1,
   Four = 2,
   Five = 3,
   Six = 4,
   Seven = 5,
   Eight = 6,
   Nine = 7,
   Ten = 8,
   Jack = 9,
   Queen = 10,
   King = 11,
   Ace = 12
};
 
struct Card
{
   const Suit   suit_;
   const Face   face_;

   Card( Suit s, Face f ) : suit_( s ), face_( f )
   {
   }
};
   
bool operator==( const Card& lhs, const Card& rhs )
{
   return lhs.suit_  == rhs.suit_ &&
          lhs.face_ == rhs.face_;
}

Now you can implement the ostream operator the following way:

#include <iostream>

std::ostream& operator<<( std::ostream& os, const Card& card )
{
   os << FaceNames[card.face_]
      << " of "
      << SuitNames[card.suit_];
  return os;
}

[exiledgolem]

Thanks GNiewerth! VERY helpful...
I'm going to need to take a closer look at it to understand it a little better.
I'm guessing I'm confused on the struct constuctor? I'm guessing that is what it is.
I remember when you create an object for that struct I need to use that constructor right? You can't use the default one?
I was trying to do a little example of my own and I was having a problem.

On another note, everyone's ideas came together great, I just need to implement the overloaded operators and I'm pretty much good to go.. There is ONE slight problem though, I believe, there SHOULD be an easier way of checking for sets... the current way of me doing it
has a slight flaw, considering the logic is only considering ONE deck... and I'm not sure if I want to add on to the program to be capable of doing more then one..

The actual problem is not in the CheckFor function that takes cards from another player or the pickup card function. When I use vector.erase, it seems that it is erasing 3/4 values... sometimes it erases all the required values, and sometimes it's just like the 1st values like all the values of hearts, just the FIRSt value of the set. It is really just the one line in the CheckForPoints function that erases stuff, and within it's context that needs to be changed... I WOULD THINK...

ALSO, it looks like the value produced by saying Vector.erase(Vector.begin()+i) is a unsigned value and I just compiled on Visual Studio prof 2008 and it gave me about 9 warnings from unsigned to signed... would that make a difference in the algorithm? just curious...
MAYBE ITTERATORS... and maybe also defining the size as a variable as a global... not really sure what to do....
is (vector.begin()+5) really valid....... .SOOo lost.

[exiledgolem]

for ANYONE thinking about using vectors, and using them to insert and erase data constantly, I suggest you using a list instead.... it seems like I found the new bug out in my code. Taken directly from some site...





There are quite a few member functions of the vector class that can potentially invalidate iterators and element references. The following shows a list of those methods and actions:

assign

clear

erase (invalidates those after the erased element)

insert

pop_back (may or may not, I am not sure)

push_back (if causes reallocation - may not when extra space for more elements reserved)

resize

reserve

swap

assignment operator=

when the vector object is destroyed


All these are capable of causing reallocations of the vector and hence are capable of invalidating iterators.





... soo DUH... list...... definately list...

[Paul McKenzie]

for ANYONE thinking about using vectors, and using them to insert and erase data constantly, I suggest you using a list instead...

Well, that isn't exactly breaking news to us here who have used C++ and STL for years.

The std::vector is basically a wrapper for a dynamic array, so it will have the same traits as any dynamic array class, where the memory used by the data must be contiguous. This means that to insert an item, erase an item, etc. the physical location of the item(s) will change. Try to write a dynamic array class where the addresses won't change and the memory must be contiguous -- you can't.

A containers such as std::list does not have to have the data "reseated" when items are inserted or removed, because that is the trait of a linked list. It's only the links of the items that need to be adjusted, and not the actual location of the item.

Regards,

Paul McKenzie

[exiledgolem]

yeah, I was mainly just saying it for the sake of the people who have been following all along. I'm suprised none of you mentioned that sooner. I know you said there were more mistakes, but nothing along the lines of oh... "your entire method of doing a deck is f'd up" I'm not upset or mad, I'm just glad I found out sooner, rather then the later part.... I'm relatively knew to programming as you can see....

Thank you though Paul.

[monarch_dodra]

yeah, I was mainly just saying it for the sake of the people who have been following all along. I'm suprised none of you mentioned that sooner. I know you said there were more mistakes, but nothing along the lines of oh... "your entire method of doing a deck is f'd up" I'm not upset or mad, I'm just glad I found out sooner, rather then the later part.... I'm relatively knew to programming as you can see....

Thank you though Paul.

I did say it actually. I didn't recommend a list, but I did tell you that your method of implementing a deck was sub-optimal.

Although quite franckly, when your vector is 7 objects long, it doesn't really matter. Also, don't get paranoid. If your inserts and erase are done at the back, then you actually get better performance than list.

PS, I did not see you use a single iterator in your entire code, so why worry about re-allocation?

[exiledgolem]

well actually monarch I didn't even know what an iterator was ... I've heard of them, I didn't know they were pretty much standard now for everything you do. Yes, you did mention from the back, and now it makes sense now to me more... I think I'm heading in the right direction finally.