ugly loop, any ideas on optimizing

snap (7)
i have this loop in my poker program that I'm writing:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
 
int determine_matches(player * the_player, game * the_game, int which_card)
{
	if (determine_two_kind(the_player->get_card(1), the_player->get_card(2)))
	{
		//cout << "your cards are the same" << endl;
		return(3);
	}
	
	for (int i = 1; i < 6; i++)
	{
		if (the_player->get_card(which_card)->get_card_number() == the_game->get_card(i)->get_card_number())
		{
			for(int j = i+1; j < 6; j++)
			{
				if(the_player->get_card(which_card)->get_card_number()  == the_game->get_card(j)->get_card_number())
				{
					return 7;
				}
				//cout << "DEBUG: in determine_two_kind, MATCH" << endl;	
			}
			return (1 + which_card);
		}
	}
	//cout << "DEBUG: In determine_two_kind, NO MATCH" << endl;
	return 0;
}


basically the code checks for two of a kind and if it finds a match then checks for 3 of kind.
it works absolutely fine, I'm just concerned that its quite an ugly bit of code, any suggestions on optimizing/ doing it a different way.

cheers.
ropez (310)
I don't think there is a performance issue here. If the code works as you intended, the only reason to "optimize" it would be to make it easier to read/manage and possibly detect bugs.

Here's one suggestion (assuming that the return type of get_card_number() is int)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
 
int determine_matches(player * the_player, game * the_game, int which_card)
{
	if (determine_two_kind(the_player->get_card(1), the_player->get_card(2)))
	{
		//cout << "your cards are the same" << endl;
		return(3);
	}

	int number = the_player->get_card(which_card)->get_card_number();
	for (int i = 1; i < 6; i++)
	{
		if (number == the_game->get_card(i)->get_card_number())
		{
			for(int j = i+1; j < 6; j++)
			{
				if(number == the_game->get_card(j)->get_card_number())
				{
					return 7;
				}
				//cout << "DEBUG: in determine_two_kind, MATCH" << endl;	
			}
			return (1 + which_card);
		}
	}
	//cout << "DEBUG: In determine_two_kind, NO MATCH" << endl;
	return 0;
}

ropez (310)
I really don't understand your return values, but this might be a possible way to simply detect if you have two or three of a kind (without the inner loop):

1
2
3
4
5
6
7
8
9
10
11
12
13
 
	int matches = 1;
	int number = the_player->get_card(which_card)->get_card_number();
	for (int i = 1; i < 6; i++)
	{
		if (number == the_game->get_card(i)->get_card_number())
		{
			matches += 1;
		}
	}
	// matches equals:
	// 1 = only the player's card
	// 2 = two of a kind
	// 3 = three of a kind etc.
snap (7)
cheers, that's a good suggestion I'm surprised i didn't think to do that.

yeah sorry i should have explained the return values a bit more, basically the function returns a unique value for each possible hand.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
 
bool determine_hand(player * the_player, game * the_game) // this function determines if there is a match in either hands.
{
	int a, b, c;
	a = determine_matches(the_player, the_game, 1);
	b = determine_matches(the_player, the_game, 2);
	c = a + b;
	// 4 possibilities, TT(5) TF(3) FT(2) FF(0)
	
	switch (c)
	{
		case 7:
			cout << the_player->get_name() << " has three of a kind" << endl;
			the_player->set_hand_number(3);
			return true;
			break;
		case 6:
			cout << the_player->get_name() << "'s cards are the same" << endl;
			the_player->set_hand_number(1);
			return true;
			break;
		case 5:
			cout << the_player->get_name() << " has two pairs" << endl;
			the_player->set_hand_number(2);
			return true;
			break;
		case 3:
			cout << the_player->get_name() << " has a pair on the 2nd card" << endl;
			the_player->set_hand_number(1);
			return true;
			break;
		case 2:
			cout << the_player->get_name() << " has a pair on the 1st card" << endl;
			the_player->set_hand_number(1);
			return true;
			break;
		case 0:
			cout << the_player->get_name() << ", unlucky you have no pairs" << endl;
			the_player->set_hand_number(0);
			return false;
			break;
		default:
			cout << "DEBUG: ERROR1 in determine_hand" << endl;
			return false;
			break;
	}
	cout << "DEBUG: ERROR2 in determine_hand" << endl;
	return false;
}


it performs determine_hand twice (once for each card in the players hand) and then adds the values together. It's an interesting way of solving things but its the first thing that came to mind.
ropez (310)
What you should do is to create some symbolic constants, so that the numbers are easier to understand>

1
2
3
4
5
 
enum Hands {
    THREE_OF_A_KIND = 7,
    TWO_PAIRS = 5,
    // ...
};


And then replace the numbers in your code with the name. It generates the same code, but looks better. You don't really need to assign numbers in the enum either, because it automatically gets 0, 1, 2...
Topic archived. No new replies allowed.