if with multiple conditions

[Kanoisa]

Hi all,

Im working on the design of a function at the moment and im not sure if the if statement i want to do is safe.

Ill start with the data structures i have as this is where the problem could come from. (simplified with relevent bits only)

class cPlayer
{
public:
   bool inRound; //indicates is the player is participating
   bool requiresAction; //indicates if the players needs to take further action this phase
   int currentBet;
   string name;

//....theres more but what i have is more than enough for this question
   
};

class cSeat //kind of a container class really
{
   bool isOccupied; //set to true when a player instance is held 
   cPlayer *player;
};

class cTable//main class of the card game
{
protected: 
   cSeat[6]; //space for 6 players
};

so as you can hopefully see from the data structures i have a table which has 6 seats. upon each seat a player can sit so it has a pointer to that player and a bool to say if the seat is occupied. simple enough i think.

The code now that worries me is this.

//section of the loop im using 
for(int x = 0; x<6; x++)
{
	if((seat[x].isOccupied==true)&&(seat[x].player->inRound==true))//if the seat is occupied and the player is still playing this round
	{
		seat[x].player->currentBet = 0;
		seat[x].player->requiresAction = true;
	}
}

Now what im concerned about is that as far as i understand the if statment should stop evaluating if the first condition is false [as it is an && operation] (ie no player occupies the seat, therefore the pointer is invalid) so it should not try to evaluate the player->inRound bit which of course would be some nasty pointer access violation.

So to put it in words now i have set the scene, is using an evaluation like this a bad idea as it could potentially cause bad memory violations, or will it be ok if the bool isOccupied is set and cleared correctly without any bugs at all times? and secondly if this is bad code or quite dangerous to do things like this can you suggest a safer way to do such a thing?

Many thanks for your help again :)

[vijayan121]

> as far as i understand the if statment should stop evaluating if the first condition is false
> [as it is an && operation] (ie no player occupies the seat, therefore the pointer is invalid)
> so it should not try to evaluate the player->inRound bit

Your understanding is correct.So it will it be completely safe if isOccupied is set and cleared correctly.

> secondly if this is bad code can you suggest a safer way to do such a thing?

It is not bad code unless it sets isOccupied incorrectly. The only extra safety that can be provided is
a. Make sure that seat[x].player is set to zero whenever the seat is not occupied
b. Provide an extra check to verify that seat[x].player is not a null pointer before you try to access seat[x].player->inRound

[mrnutty]

You should be fine, but you can always separate your logic :

if(condition1 && condition2){
  /*logic here */
}
if(condition1){
  if(condition2){
     /* logic here */
  }
}

But,you don't even need isOccupied variable, if you default initialize player to
null, in the class, then you can just check if seat[x].player == NULL.

But I see you are using raw arrays, just use vectors instead.

Also, maybe some context would help so we can help better the design. For example,
I question why inRound and requireAction as a member variable. Those name suggest
to me that, thats the responsibility of some other class. Remember to minimize
the things the classes needs to do, if possible.

[Kanoisa]

Thanks for replies, ill add the c/pseudo code rewrite of the algorithym iv been working on to put things in context better. But in short the reason for is occupied as that a player can be in the seat but not playing the round (ie they folded)

the complete code for this function is listed below. please note iv been doing it in notepad++ not my actual project as i wanted to focus just on this algorithym so it may have some mistakes in typing or the likes.

char action;
bool betting = true;
int raise;
int firstPlayer++;//increment the first player in this round
int betOnTable = 0;
cSeat *curSeat;
cPlayer *player;


//init players per beting cycle
for(int x = 0; x<6; x++)
{
	if((seat[x].isOccupied==true)&&(seat[x].player.inRound==true))//if the seat is occupied and the player is still playing this round
	{
		seat[x].player->currentBet = 0;
		seat[x].player->requiresAction = true;
	}
}

while(betting)
{

	int playersRemaining = 0;
	for(int x = firstPlayer; x<(frstPlayer+6); x++)//loop through all the players
	{
		curSeat = seat[x%6];//get refrence to the seat of interest
		player = curSeat->player;
		
		if(curSeat->isOccupied)//seat holds a player who is playing
		{
			if(player->requiresAction)
			{
				action = getPlayerDesiredAction(player,betOnTable) //check, call,raise or fold
				switch(action)
				{
					case(check):
					case(call):
							if(betOnTable!=0)//they cant check if the bet is 0 they have to call
							{
								if(!player->DeductCredit(betOnTable-(player->currentBet)))//they cannot afford the bet
								{
									//player is broke so remove them
									table.removePlayer(curSeat)
								}
								else
								{
									//player can afford bet
									player->currentBet = betOnTable;//record how much they have bet so far
									playersRemaining++;
								}
							}
							else
							{
								//player checks for now
								playersRemaining++;
							}
							break;
							
					case(raise):
							raise = getPlayerRaise(player);
							
							if(!player->DeductCredit((betOnTable+raise)-player->currentBet))//they cannot afford the bet
							{
								//cannot afford the raise
								cout<<"You cannot afford this raise"<<endl;
							}
							else
							{
								//player can afford raise
								betOnTable +=raise;//increase the bet on the table
								player->currentBet = betOnTable;//record how much they have bet so far
								playersRemaining++;
							}
							break;
							
					case(fold):
							cout<<"Player: "<<player->name<<" folds"<<endl;
							player->inRound = false;
							break;
					
				}//end switch
				curSeat->requiresAction = false;
			}
		}
	}
	//all players have done something so now check if all players have reached the same bet
	for(int x = 0; x<6; x++)//loop through all players;
	{
		if((seat[x].isOccupied==true)&&(seat[x].player.inround==true))//if the seat is occupied and the player is still playing this round
		{
			if(seat[x].player->currentBet!=betOnTable)//if the players bet is too low
			{
				seat[x]->player->requiresAction = true;//mark player as requiring further action
			}
			else
			{
				playersRemaining --; //one less player to deal with
			}
		}
	}
	
	if(playersRemaining == 0)
	{
		//all players have matched the bet
		betting = false;
	}
}

basically what is happening is that for each seat
if there is an active player they need to choose what to do this phase.
if they check or call/ neccessary funds are deducted from thier account.
if they raise the total neccessary is deducted from this account to match the bet on the table
if they fold they do not have to pay anything and will not play again untill the end of the cards hand (round).

towards the end it takes the number of players who (checked/called or raised) and then sees if the same amount of playes have matched the bet on the table. if all players have matched it the job is done if not we have to go around again untill all the bets match.