Poker Game: Refactor method to evaluate poker hands

Question

Preface: I've perused the other related questions before creating this one but I believe that those solutions weren't applicable to my situation. I'll concede that its possible that I may not have understood a possible solution (if any).

I'm working on refactoring an old poker game that I created, which is a .NET Framework console application, and I came across this method.

    /// <summary>
    /// Evaluates the value of the player hand and computer hand. 
    /// </summary>
    /// <param name="pComputerHand">The computer's current hand.</param>
    /// <param name="pPlayerHand">The player's current hand.</param>
    /// <returns>Returns true if the player won and false if the player lost. This value is stored in the the bool won variable.</returns>
    private static bool CompareHands(SuperCard[] pComputerHand, SuperCard[] pPlayerHand)
    {
        // Stores the value of the player and computer hands.
        int playerHandValue = 0; 
        int computerHandValue = 0;

        // Evaluates the value of computer hand
        foreach(SuperCard card in pComputerHand)
        {
            computerHandValue += (int)card.CardRank;
        }

        // Evaluates the value of player hand
        foreach(SuperCard card in pPlayerHand)
        {
            playerHandValue += (int)card.CardRank;
        }

        // If there is a royal flush in the computer's hand and the player's hand, the player lost.
        if (PokerHands.RoyalFlush(pPlayerHand) & PokerHands.RoyalFlush(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a royal flush!");
            return false;
        }

        // If there is a royal flush in the player's hand, the player won.
        if (PokerHands.RoyalFlush(pPlayerHand))
        {
            Console.WriteLine("\nYou have a royal flush!");
            return true;
        }

        // If there is a royal flush in the computer's hand, the player lost.
        if (PokerHands.RoyalFlush(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a royal flush!");
            return false;
        }

        #region Straight Flush
        // If there is a straight flush in the computer's hand and the player's hand, the player lost.
        if (PokerHands.StraightFlush(pPlayerHand) & PokerHands.StraightFlush(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a straight flush!");
            return false;
        }

        // If there is a straight flush in the player's hand, the player won.
        if (PokerHands.StraightFlush(pPlayerHand))
        {
            Console.WriteLine("\nYou have a straight flush!");
            return true;
        }

        // If there is a straight flush in the computer's hand, the player lost.
        if (PokerHands.StraightFlush(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a straight flush!");
            return false;
        }
        #endregion


        #region Four Of A Kind 
        // If there is a four of a kind in the computer's hand and the player's hand, the player lost.
        if (PokerHands.FourOfAKind(pPlayerHand) & PokerHands.FourOfAKind(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a four of a kind!");
            return false;
        }

        // If there is a four of a kind in the player's hand, the player won.
        if (PokerHands.FourOfAKind(pPlayerHand))
        {
            Console.WriteLine("\nYou have a four of a kind!");
            return true;
        }

        // If there is a four of a kind in the computer's hand, the player lost.
        if (PokerHands.FourOfAKind(pComputerHand))
        {
            Console.WriteLine("\nThe computer has four of a kind!");
            return false;
        }
        #endregion


        #region Full House
        // If there is a full house in the computer's hand and the player's hand, the player lost.
        if (PokerHands.FullHouse(pPlayerHand) & PokerHands.FullHouse(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a full house!");
            return false;
        }

        // If there is a full house in the player's hand, the player won.
        if (PokerHands.FullHouse(pPlayerHand))
        {
            Console.WriteLine("\nYou have a full house!");
            return true;
        }

        // If there is a four of a kind in the computer's hand, the player lost.
        if (PokerHands.FullHouse(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a full house!");
            return false;
        }
        #endregion


        #region Flush
        // If there is a flush in the player hand and the computer hand, the player lost.
        if (PokerHands.Flush(pPlayerHand) && PokerHands.Flush(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a flush!");
            return false;
        }

        // If there is a flush in the player hand, the player won.
        if (PokerHands.Flush(pPlayerHand))
        {
            Console.WriteLine("\nYou have a flush!");
            return true;
        }

        // If there is a flush in the computer hand, the player lost.
        if (PokerHands.Flush(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a flush!");
            return false;
        }
        #endregion


        #region Straight
        // If there is a straight in the computer's hand and the player's hand, the player lost.
        if (PokerHands.Straight(pPlayerHand) & PokerHands.Straight(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a straight!");
            return false;
        }

        // If there is a straight  in the player's hand, the player won.
        if (PokerHands.Straight(pPlayerHand))
        {
            Console.WriteLine("\nYou have a straight!");
            return true;
        }

        // If there is a straight in the computer's hand, the player lost.
        if (PokerHands.Straight(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a straight!");
            return false;
        }
        #endregion


        #region Three Of A Kind
        // If there is a three of a kind in the computer's hand and the player's hand, the player lost.
        if (PokerHands.ThreeOfAKind(pPlayerHand) & PokerHands.ThreeOfAKind(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a three of a kind!");
            return false;
        }

        // If there is a three of a kind  in the player's hand, the player won.
        if (PokerHands.ThreeOfAKind(pPlayerHand))
        {
            Console.WriteLine("\nYou have a three of a kind!");
            return true;
        }

        // If there is a three of a kind in the computer's hand, the player lost.
        if (PokerHands.ThreeOfAKind(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a three of a kind!");
            return false;
        }
        #endregion


        #region Two Pair
        // If there is a two pair in the computer's hand and the player's hand, the player lost.
        if (PokerHands.TwoPair(pPlayerHand) & PokerHands.TwoPair(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a two pair!");
            return false;
        }

        // If there is a two pair  in the player's hand, the player won.
        if (PokerHands.TwoPair(pPlayerHand))
        {
            Console.WriteLine("\nYou have a two pair!");
            return true;
        }

        // If there is a two pair in the computer's hand, the player lost.
        if (PokerHands.TwoPair(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a two pair!");
            return false;
        }
        #endregion


        #region One Pair
        // If there is a one pair in the computer's hand and the player's hand, the player lost.
        if (PokerHands.OnePair(pPlayerHand) & PokerHands.OnePair(pComputerHand))
        {
            Console.WriteLine("\nBoth players have a one pair!");
            return false;
        }

        // If there is a one pair  in the player's hand, the player won.
        if (PokerHands.OnePair(pPlayerHand))
        {
            Console.WriteLine("\nYou have a one pair!");
            return true;
        }

        // If there is a one pair in the computer's hand, the player lost.
        if (PokerHands.OnePair(pComputerHand))
        {
            Console.WriteLine("\nThe computer has a one pair!");
            return false;
        }
        #endregion

        // If the player's hand value is greater than the computer's hand value, the player won.
        if (playerHandValue > computerHandValue)
        {
            return true;
        }

        // If the player's hand value is less than or equal to the computer's hand value, the player lost.
        if (playerHandValue <= computerHandValue)
        {
            return false;
        }

        // Satisfies all code paths must return a value error.
        else
        {
            return false;
        }


    } // end of CompareHands()     

This is what the class diagram for the project looks like:

This is what the console output looks like:

My question is whether its possible to extract a method (or several) out of these if statements, and provided that it is possible, how would I go about doing so? I also appreciate any other advice that you can offer like the regions being bad, for example.

Answer 1

Addressing the immediate question

I cannot comment on the coding itself, not being fluent in C#.

The old code makes it difficult to directly extract suitable method because the value of the hand is intrinsically tied into the class PokerHands (for which we cannot see the code). Note that I said "... the value ..." instead of "... the calculation of the value ..." - this is an important distinction.

To do what you want (and to make it easier to maintain later in life), the PokerHands class should be restructured to return a value instead of a Boolean. In other words, it should, as currently used in your diagram, be an enum. But, running with the concept of a class:

e.g playerHandValue = PokerHands.HandValue(pPlayerHand) where HandValue returns an enum that is ordered in the order of outcomes (e.g. pair, triple ... royal flush).

This small change will mean the above code only checks if playerHandValue > computerHandValue or similar. If the two values are the same, then you can have some other method that checks ranking (e.g. doubles base don pair value, or flushes based on suit etc.)

This line of thinking leads to a stronger restructure - that is a Hand Class that encapsulates the above thinking, where the properties returned are:

• Combination value
• High card value for a given combination
• High suit value for a given combination

Your CompareHands method then doesn't have to worry about calculating any of the values, it only has to compare them - falling more in line with the SOLID OOP principles.

Addressing the underlying concern

Now that I have had a closer look at your class diagram, I can see that you will have continued problems trying to re-work this program.

• The breakdown of cards into different classes by suit will create problem of repetition (DRY) and cross checking.
• The lack of a "Hand" as an object to me worked with is creating the problem you are trying to address above.
• The lack of a "Deck" (card set is the closest) as an object will create further problems.

I recommend that any early work focusses on the structure, this may help prevent any other problems as you work your way through.

Answer 2

One simple refactoring for this method could be to simplify the following code block, that is used 9 times in the Method.

    // If there is a royal flush in the computer's hand and the player's hand, the player lost.
    if (PokerHands.RoyalFlush(pPlayerHand) & PokerHands.RoyalFlush(pComputerHand))
    {
        Console.WriteLine("\nBoth players have a royal flush!");
        return false;
    }

    // If there is a royal flush in the player's hand, the player won.
    if (PokerHands.RoyalFlush(pPlayerHand))
    {
        Console.WriteLine("\nYou have a royal flush!");
        return true;
    }

    // If there is a royal flush in the computer's hand, the player lost.
    if (PokerHands.RoyalFlush(pComputerHand))
    {
        Console.WriteLine("\nThe computer has a royal flush!");
        return false;
    }

To simplify this code block, you could add the following enum and methods:

private enum Winner{ None, Both, Player, Computer }

private static Winner GetWinnerForHand(bool computerHandCheckResult, bool playerHandCheckResult)
{
    if (computerHandCheckResult && playerHandCheckResult)
        return Winner.Both;

    if (playerHandCheckResult)
        return Winner.Player;

    if (computerHandCheckResult)
        return Winner.Computer;

    return Winner.None;
}

private static void OutputWinnerToConsole(Winner winner, string handText)
{
    if (winner == Winner.Both)
        Console.WriteLine(String.Format("\nBoth players have a {0}!", handText));

    if (winner == Winner.Player)
        Console.WriteLine(String.Format("\nYou have a {0}!", handText));

    if (winner == Winner.Computer)
        Console.WriteLine(String.Format("\nThe computer has a {0}!", handText));
}

Then you can refactor the code block to:

    // Check for royal flush
    var winner = GetWinnerForHand(PokerHands.RoyalFlush(pComputerHand), PokerHands.RoyalFlush(pPlayerHand));
    if (winner != Winner.None)
    {
        OutputWinnerToConsole(winner, "royal flush");
        return winner == Winner.Player;
    }

You can get rid of most of the comments because the methods explain, what is happening. You can go further by modifing the GetWinnerForHand to

private static Winner GetWinnerForHand(Func<SuperCard[], bool> check, SuperCard[] computerHand, SuperCard[] playerHand)
{
    var computerHandCheckResult = check(computerHand);
    var playerHandCheckResult = check(playerHand);
    if (computerHandCheckResult  && playerHandCheckResult)
        return Winner.Both;

    if (playerHandCheckResult)
        return Winner.Player;

    if (computerHandCheckResult )
        return Winner.Computer;

    return Winner.None;
}

Then you can refactor the code block to:

    // Check for royal flush
    var winner = GetWinnerForHand((hand) => PokerHands.RoyalFlush(hand), pComputerHand, pPlayerHand);
    if (winner != Winner.None)
    {
        OutputWinnerToConsole(winner, "royal flush");
        return winner == Winner.Player;
    }

With this change, you can even move the checks in a foreach loop, if you put the expression and handText to a list.

Answer 3

Review

The other answers already provide proper ways to refactor the design and split the method up into smaller methods. I would like to address some minor concerns I have with this method.

• The method name CompareHands suggests a comparison, which in C# has a built-in pattern IComparable<SuperCard> yielding an integer containing the comparison result, rather than a bool.
• Its description "Evaluates the value of the player hand and computer hand." doesn't match its name (evaluation vs comparison).
• The documentation of the return value is verbose, but doesn't inform us how a tie is treated "Returns true if the player won and false if the player lost. This value is stored in the the bool won variable.".
• I believe the names pComputerHand and pPlayerHand are some sort of variant of Hungarian Notation (p stands for parameter?), which is not the default naming convention in C#. Prefer using computerHand and playerHand.
• playerHandValue and computerHandValue are calculated upfront, but their values are only used at the end of the method. In many cases, early exit prevents their usability. Avoid performing logic that may or may not be required. Moving this code to the end of the method is a better option.
• Inline documentation should yield additional information about the code. In general, I would avoid using too much inline documentation. You'd no longer be able to differentiate between obvious and important comments. Also, code should be self-explaining so most of your comments are redundant: for instance // Stores the value of the player and computer hands. -> yes, we can see that the next line :)
• I believe you missed a region #region RoyalFlush. However, when you require many regions inside a method (even one might be too many), you might need to think about refactoring the method and split it up into multiple methods, or even classes using some design pattern. The other answers already talk about how to split up your method and use reusable helper methods. One other way you could go about business is to use the Chain of Responsibility Pattern to go through hand types and yielding early when no tie is found: royal flush -> next: straight flush -> next: four of a kind -> and so on.. Each node in this chain could be a class with a pointer to the next node.
• This method does 3 separate things: (1) returns the winner (2) evaluates which hand the winner holds (3) renders output to the console. I would split up these 3 concerns in 3 different methods to have Separation of Concerns.
• If performance is important, you could create and reuse sub-routines. For instance, why would you calculate a Flush naively, if you've already checked for Royal Flush? The latter could store whether either Flush or Straight are found.