2
\$\begingroup\$

I wrote a little program that recognises poker hand combinations and compares hands if the ranks of both hands are the same. I had tested my program on possible combinations for each rank and the outcomes were correct. I would like to know how it is possible to improve the design of my code and maybe cut a few steps.

I am also planning on expanding the whole program a bit further e.g. recognising the best combination from two initial cards and five cards on the table, so making code programmer-friendly is a priority for me.

vector<string> cards{"6H", "7H", "8H", "9H", "TH"}; //Or any other hand

char rank = 0;
vector<char> vals;

//Extracting values of each card without a suit
for(vector<string>::iterator it = cards.begin(); it != cards.end(); ++it)
{
    if((*it)[0] == 'T')vals.push_back(58);
    else if ((*it)[0] == 'J')vals.push_back(59);
    else if ((*it)[0] == 'Q')vals.push_back(60);
    else if ((*it)[0] == 'K')vals.push_back(61);
    else if ((*it)[0] == 'A')vals.push_back(62);
    else vals.push_back((*it)[0]);
}

// Checking for pairs, triples and quadruples
sort(vals.begin(), vals.end(), greater<char>());
for(vector<char>::iterator it = vals.begin(); it != vals.end(); it)
{
    int counter = 0;
    auto nx = next(it);
    while(*it == *nx && nx != vals.end())
    {
        ++counter;
        ++nx;
    }
    switch(counter)
    {
        case 1:
            if(rank == 0)
            {
                rank = 1;
                rotate(vals.begin(), it, nx); //Pairs are compared first
            }
            else if(rank == 1)
            {
                rank = 2;
                rotate(vals.begin()+2, it, nx); //2nd pairs are compare after 1st pairs
            }
            else if(rank == 3)rank = 6;
            break;
        case 2:
            if(it != vals.begin())rotate(vals.begin(), it, nx); //Tripples are always compared first
            if(rank == 0) rank = 3;
            else if(rank == 1)rank = 6; //Full House
            break;
        case 3:
            rank = 7;
            if(it != vals.begin())rotate(vals.begin(), it, nx);
    } 
    it = nx;
}

//Checking for flash and street
if(cards[0][1] == cards[1][1] && cards[1][1] == cards[2][1] && cards[2][1] == cards[3][1] && cards[3][1] == cards[4][1])rank = 5;
if(vals[0] == vals[1]+1 && vals[1] == vals[2]+1 && vals[2] == vals[3]+1 && vals[3] == vals[4]+1)
{ 
    if(rank == 0)rank = 4;
    else rank = 8;
}
cout << "RANK: " << static_cast<int>(rank) << endl;
\$\endgroup\$
2
\$\begingroup\$
  • Overall, I can't quite tell how this is supposed to be structured. It looks like it all belongs to a single function which has been omitted. In either case, different things are happening here, so they should be separated into additional functions as such.

    For instance, each section you've already preceded with a comment should have a function. They may all do card operations, but their purposes still differ. If someone needed to find a certain area to examine, then that would take more time to find, even with comments.

  • Since you're already using C++11, you can change all of your vector::iterators to auto, which will help keep your code simpler.

    Better yet, you can just use a range-based for loop:

    for (auto& it : cards) {}
    
  • This isn't very idiomatic:

    (*it)[0]
    

    Since this vector contains std::strings, you can use std::string::front() to acquire the first character from an element:

    it.front();
    

    Note: the . operator here may only work if you follow my second suggestion. Otherwise, you may need to use the -> operator.

  • This is really hard to read and maintain:

    if(cards[0][1] == cards[1][1] && cards[1][1] == cards[2][1] && cards[2][1] == cards[3][1] && cards[3][1] == cards[4][1])rank = 5;
    

    I had to scroll all the way to the right to see that it does indeed perform an action. While it's okay to have if statements on a single line, doing so with a long line is a bad idea. In this case, put the rank = 5; on a separate line with curly braces, just like with your other code.

  • This is also hard to read:

    if(vals[0] == vals[1]+1 && vals[1] == vals[2]+1 && vals[2] == vals[3]+1 && vals[3] == vals[4]+1)
    

    With this many conditions, you should at least list them vertically in some way so that each one is clearly visible.

    (This advice also applies to the previous bulletpoint.)

    Moreover, do keep in mind that long vertical sections aren't as bad as long horizontal sections. If, say, someone were to read this on a terminal with line-wrapping, then your code's readability for them would decrease even more.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.