Derived methods with Poker classes

Question

I have an interface with two functions. This interface is being inherited by an abstract class which implements just one of those functions. The other one is left to be implemented by the derived classes of the abstract class.

public interface ICombination
{
    bool Check(IReadOnlyList<int> hand,UsersProperties user);
    void UpdateHand(UsersProperties user, int combinationType, int powerType);
}

public abstract class ICombinationAnalyzer : ICombination
{
    public abstract bool Check(IReadOnlyList<int> hand, UsersProperties user);

    public void UpdateHand(UsersProperties user, int combinationType, int powerType)
    {
        user.Type = combinationType;
        user.Power = powerType + user.Type * 100;
        MainPoker.Win.Add(new Hand { Power = user.Power, Current = user.Type });
    }
}

Now the problem is that the derived classes implement a lot of variables and those variables are being instantiated the same way each time:

public class StraightFlush : ICombinationAnalyzer
{
    public override bool Check(IReadOnlyList<int> hand, UsersProperties user)
    {
        int _kicker,
            _rightCard,
            _sumOfHands,
            _leftCard;
        int[] straight1 = new int[5];
        int[] straight = new int[7];
        int k = (int)MainPoker.TableCards.FirstCard;
        straight[0] = hand[user.RightCard];
        straight[1] = hand[user.LeftCard];
        for (int j = 2; j < straight.Length; j++)
        {
            straight[j] = hand[k];
            k++;
        }
        k = (int)MainPoker.TableCards.FirstCard;
        for (int j = 0; j < straight1.Length; j++)
        {
            straight1[j] = hand[k];
            k++;
        }
        int[] a = straight.Where(o => o % 4 == (int)Cards.CardSuits.Club).ToArray();
        int[] b = straight.Where(o => o % 4 == (int)Cards.CardSuits.Diamond).ToArray();
        int[] c = straight.Where(o => o % 4 == (int)Cards.CardSuits.Heart).ToArray();
        int[] d = straight.Where(o => o % 4 == (int)Cards.CardSuits.Spade).ToArray();
        int[] st1 = a.Select(o => o / 4).Distinct().ToArray();
        int[] st2 = b.Select(o => o / 4).Distinct().ToArray();
        int[] st3 = c.Select(o => o / 4).Distinct().ToArray();
        int[] st4 = d.Select(o => o / 4).Distinct().ToArray();

        int[][] combinedArrays = { st1, st2, st3, st4 };
        Array.Sort(straight);
        Array.Sort(st1);
        Array.Sort(st2);
        Array.Sort(st3);
        Array.Sort(st4);
        if (hand[user.RightCard] / 4 != (int)Cards.CardTypes.Ace &&
            hand[user.LeftCard] / 4 != (int)Cards.CardTypes.Ace)
        {
            if (hand[user.RightCard] / 4 > hand[user.LeftCard] / 4)
            {
                _kicker = hand[user.RightCard] / 4;
            }
            else
            {
                _kicker = hand[user.LeftCard] / 4;
            }
        }
        if (hand[user.RightCard] / 4 == (int)Cards.CardTypes.Ace)
        {
            _rightCard = 13;
            _kicker = 13;
        }
        else
        {
            _rightCard = hand[user.RightCard] / 4;
        }
        if (hand[user.LeftCard] / 4 == (int)Cards.CardTypes.Ace)
        {
            _leftCard = 13;
            _kicker = 13;
        }
        else
        {
            _leftCard = hand[user.LeftCard] / 4;
        }
        _sumOfHands = _rightCard + _leftCard;
        const int localType = (int)Hand.Combinations.FourOfAKind;
        for (int j = 0; j <= 3; j++)
        {
            if (straight[j] / 4 == straight[j + 1] / 4 && straight[j] / 4 == straight[j + 2] / 4 &&
                straight[j] / 4 == straight[j + 3] / 4)
            {
                UpdateHand(user, localType, straight[j] / 4 * 4);
                return true;
            }
            if (straight[j] / 4 == (int)Cards.CardTypes.Ace && straight[j + 1] / 4 == (int)Cards.CardTypes.Ace &&
                straight[j + 2] / 4 == (int)Cards.CardTypes.Ace && straight[j + 3] / 4 == (int)Cards.CardTypes.Ace)
            {
                UpdateHand(user, localType, 13 * 4);
                return false;
            }
        }
        return false;
    }
}

And the second derived class :

    public class RoyalFlush : ICombinationAnalyzer
{
    public override bool Check(IReadOnlyList<int> hand, UsersProperties user)
    {
        int _kicker,
            _rightCard,
            _sumOfHands,
            _leftCard;
        int[] straight1 = new int[5];
        int[] straight = new int[7];
        int k = (int) MainPoker.TableCards.FirstCard;
        straight[0] = hand[user.RightCard];
        straight[1] = hand[user.LeftCard];
        for (int j = 2; j < straight.Length; j++)
        {
            straight[j] = hand[k];
            k++;
        }
        k = (int) MainPoker.TableCards.FirstCard;
        for (int j = 0; j < straight1.Length; j++)
        {
            straight1[j] = hand[k];
            k++;
        }
        int[] a = straight.Where(o => o%4 == (int) Cards.CardSuits.Club).ToArray();
        int[] b = straight.Where(o => o%4 == (int) Cards.CardSuits.Diamond).ToArray();
        int[] c = straight.Where(o => o%4 == (int) Cards.CardSuits.Heart).ToArray();
        int[] d = straight.Where(o => o%4 == (int) Cards.CardSuits.Spade).ToArray();
        int[] st1 = a.Select(o => o/4).Distinct().ToArray();
        int[] st2 = b.Select(o => o/4).Distinct().ToArray();
        int[] st3 = c.Select(o => o/4).Distinct().ToArray();
        int[] st4 = d.Select(o => o/4).Distinct().ToArray();

        int[][] combinedArrays = {st1, st2, st3, st4};
        Array.Sort(straight);
        Array.Sort(st1);
        Array.Sort(st2);
        Array.Sort(st3);
        Array.Sort(st4);
        if (hand[user.RightCard]/4 != (int) Cards.CardTypes.Ace &&
            hand[user.LeftCard]/4 != (int) Cards.CardTypes.Ace)
        {
            if (hand[user.RightCard]/4 > hand[user.LeftCard]/4)
            {
                _kicker = hand[user.RightCard]/4;
            }
            else
            {
                _kicker = hand[user.LeftCard]/4;
            }
        }
        if (hand[user.RightCard]/4 == (int) Cards.CardTypes.Ace)
        {
            _rightCard = 13;
            _kicker = 13;
        }
        else
        {
            _rightCard = hand[user.RightCard]/4;
        }
        if (hand[user.LeftCard]/4 == (int) Cards.CardTypes.Ace)
        {
            _leftCard = 13;
            _kicker = 13;
        }
        else
        {
            _leftCard = hand[user.LeftCard]/4;
        }
        _sumOfHands = _rightCard + _leftCard;

        foreach (var t in combinedArrays.Where(t => t.Length >= 5))
        {
            if (t[0] + 4 == t[4])
            {
                UpdateHand(user, (int) Hand.Combinations.StraightFlush, t.Max()/4);
            }
            if (t[0] != (int) Cards.CardTypes.Ace || !t.Contains((int) Cards.CardTypes.Ten) ||
                !t.Contains((int) Cards.CardTypes.Jack) || !t.Contains((int) Cards.CardTypes.Queen) ||
                !t.Contains((int) Cards.CardTypes.King)) continue;
            UpdateHand(user, (int) Hand.Combinations.RoyalFlush, t.Max()/4);
            return true;
        }
        return false;
    }
}

As you can see, all the variables have the exact same declaration and the exact same values. However, they do depend on the input parameter UserProperties. I can probably declare them somewhere in the abstract class, but I will still have to have a method for it.

Is this the best way to do it? There are more derived classes that declare the same variables, so it's a big mess. In some of the derived classes, _kicker and sumOfHand variables are not being used. Any improvements to the declaration of the variables is also appreciated.

Answer 1

I have no knowledge of poker, and perhaps that's why I find the code confusing, so I won't comment on reducing the code duplication. I'll comment on what's confusing me from my non-poker-understanding perspective, which may be useful because I should at least be able to have a decent understanding of what you're trying to achieve, which I don't.

int[] straight1 = new int[5];
int[] straight = new int[7];

"straight" and "straight1"? Maybe a single variable called "straight" is ok, but unless "straight1" is a poker term you should make it mean something. Stuff like var1, var2 etc doesn't tell me the purpose of the variable.

int k = (int)MainPoker.TableCards.FirstCard;

"k" tells me nothing about what its purpose is.

for (int j = 2; j < straight.Length; j++)
{
    straight[j] = hand[k];
    k++;
}
k = (int)MainPoker.TableCards.FirstCard;
for (int j = 0; j < straight1.Length; j++)
{
    straight1[j] = hand[k];
    k++;
}

What's that? Put it in a separate method and give it a descriptive name, so people know what that code is doing (from an intention perspective).

int[] a = straight.Where(o => o % 4 == (int)Cards.CardSuits.Club).ToArray();
int[] b = straight.Where(o => o % 4 == (int)Cards.CardSuits.Diamond).ToArray();
int[] c = straight.Where(o => o % 4 == (int)Cards.CardSuits.Heart).ToArray();
int[] d = straight.Where(o => o % 4 == (int)Cards.CardSuits.Spade).ToArray();
int[] st1 = a.Select(o => o / 4).Distinct().ToArray();
int[] st2 = b.Select(o => o / 4).Distinct().ToArray();
int[] st3 = c.Select(o => o / 4).Distinct().ToArray();
int[] st4 = d.Select(o => o / 4).Distinct().ToArray();

int[][] combinedArrays = { st1, st2, st3, st4 };
Array.Sort(straight);
Array.Sort(st1);
Array.Sort(st2);
Array.Sort(st3);
Array.Sort(st4);

You can combine some of the linq expressions to do the same in fewer statements, but again this code should be farmed out to a separate method with a descriptive name which tells me what you're trying to accomplish.

if (hand[user.RightCard] / 4 != (int)Cards.CardTypes.Ace && hand[user.LeftCard] / 4 != (int)Cards.CardTypes.Ace)

does not tell me as much as

var handContainsAce = !(hand[user.RightCard] / 4 != (int)Cards.CardTypes.Ace && hand[user.LeftCard] / 4 != (int)Cards.CardTypes.Ace) if (handContainsAce)

(Although I just made up "handContainsAce", I have no idea if that's really what that bit of code means.) I know some people don't like "temp variables", but I find them useful (especially with bools) for describing the purpose of an otherwise unclear bit of logic.

Same for pretty much all the rest of your if statements, especially if (t[0] != (int) Cards.CardTypes.Ace || !t.Contains((int) Cards.CardTypes.Ten) || !t.Contains((int) Cards.CardTypes.Jack) || !t.Contains((int) Cards.CardTypes.Queen) || !t.Contains((int) Cards.CardTypes.King)). Maybe that even needs to move into its own method.

_rightCard = 13;
_kicker = 13;

What does 13 mean? I assume it has a special meaning otherwise you wouldn't have hardcoded the number, so set it as a const somewhere and then use the descriptive const name in the code.

const int localType = (int)Hand.Combinations.FourOfAKind;

It seems a bit weird to declare a const in the middle of your method.

foreach (var t in combinedArrays.Where(t => t.Length >= 5))

Why should the length >= 5? (Rhetorical question; the code should tell me.)

Overall the code is very procedural and not object oriented at all, and your method contains way too much code (that's a subjective evaluation, but anyway) and does too much. Bits of logic should be separated out into their own methods and given descriptive names so anyone reading the code can understand what you're trying to accomplish from a higher level perspective, without having to read all the lower level code to figure that out. And variable names should also be descriptive.

Answer 2

From the accepted answer:

I have no knowledge of poker, and perhaps that's why I find the code confusing, so I won't comment on reducing the code duplication

I'll give it a shot ==> Re-conceptualize the program:

• A Player has a Hand
• The Hand is evaluated to determine what (s)he has
• Perhaps Hand class has a property that tells what it is: pair, straight, etc.
• A Hand is 5 Card objects
• A Card consists of a value and a suit.

The mass of arrays is essentially capturing the information, or context if you will, that will instead be defined as appropriate properties in the appropriate classes.

// I like the default to be "undefined". It does not make sense
// that every `new Card` will be the ace of spades, for example.
// Forcing setting explicit values is less error prone IMHO.
public ValueEnum { Undefined, Pair, Straight, RoyalFlush ... }
public SuitEnum  { Undefined, Spades, Diamonds, Clubs, Hearts }

public class Hand {
    public Card[] theCards {get; protected set;}
    public ValueEnum Value {get; protected set;}

    public override string ToString() {
        // I predict comments saying to use a StringBuilder.

       string myHand = string.Empty;

       foreach(Card card in theCards)
           myHand = string.Format("{0}{1}\n",myHand, card); //Card.ToString() is inferred

       return myHand;
    }
}

public class Card {
    public SuitEnum TheSuit {get; set;}
    public ValueEnum TheValue {get; set;}

    public override string ToString(){
        return string.Format("{0} of {1}", TheValue, TheSuit);
}

//perhaps inside a separate "hand evaluator" class
EvalPlayerHand(Hand playerHand) {} // sets Hand.Value

// OR static method(s) in the Hand class
Hand.EvalPlayerHand(Hand playerHand) {}