2
\$\begingroup\$

Part 1: Beginnings of a Poker hand classifier Part 2: Beginnings of a Poker hand classifier part 2

I'm a beginner programmer, working on learning c# and object orientation and have been working on a poker hand classifier for practice. I've expanded my class since last part and made some decisions that i would really like feedback on.

Note: I May not have done all the ideas suggested to me, This was either because I had to make changes to classes/method and the ideas no longer worked or it was a bit beyond me.

Deck Object class and 2 hand class inheriting hand

 public class Deck
{
    static Random rand = new Random();
    public List<Card> Cards { get; private set; }
    public Deck()
    {
        Cards = new List<Card>();
        FillDeck(this);
    }
    private static void FillDeck(Deck deck)
    {
        Face[] faceNames = { Face.Ace, Face.Two, Face.Three, Face.Four, Face.Five, Face.Six, Face.Seven, Face.Eight, Face.Nine, Face.Ten, Face.Jack, Face.Queen, Face.King };
        Suit[] allSuits = { Suit.Diamond, Suit.Club, Suit.Heart, Suit.Spade };

        foreach (Suit suit in allSuits)
        {
            foreach (Face face in faceNames)
            {
                deck.Cards.Add(new Card(suit, face));
            }
        }
        ShuffleDeck(deck);
    }
    public static void ShuffleDeck(Deck deck)
    {    
        for (int i = 1; i <= deck.Cards.Count; i++)
        {
            PerformCardSwap(deck);
        }
    }
    private static void PerformCardSwap(Deck deck)
    {
        int randomIndex = rand.Next(1, deck.Cards.Count);
        Card firstCard = deck.Cards[0];
        Card randomCard = deck.Cards[randomIndex];
        deck.Cards[0] = randomCard;
        deck.Cards[randomIndex] = firstCard;
    }
    public static List<Card> Deal5Cards(Deck deck)
    {
        List<Card> newHand = new List<Card>();
        for (int i = 0; i < 5; i++)
        {
            Card temp = deck.Cards[0];
            newHand.Add(temp);
            deck.Cards.RemoveAt(0);
        }
        return newHand;

    }
    public static List<Card> Deal2Cards(Deck deck)
    {
        List<Card> newHand = new List<Card>();
        for (int i = 0; i < 2; i++)
        {
            Card temp = deck.Cards[0];
            newHand.Add(temp);
            deck.Cards.RemoveAt(0);
        }
        return newHand;

    }
}
public class TwoCardHand : Hand
{
    public new List<Card> Cards { get; }
    public TwoCardHand(List<Card> cards)
    {
        if (cards.Count != 2)
        {
            throw new ArgumentOutOfRangeException("Invalid amount of Cards");
        }
        Cards = new List<Card>(cards);

    }
    public TwoCardHand()
    {
        Cards = Deck.Deal2Cards(deck);
    }
}

Is this the best way to "shuffle the deck"? I seem to have a lot of diamond suits by chance during testing. Inheriting Hand was required to ensure they have the same static deck object property so they did not draw the same cards. Is this bad programming, I Couldn't think of another way to do this...

Hand Object - a lot of changes especially inside compare to. Which now sorts pairing value correctly and provides the best possible hand from 21 sets of hands.

 public class Hand : IComparable<Hand>
{
    public static Deck deck = new Deck();
    public List<Card> Cards { get; private set; }
    public PokerHandsRank HandRank
    {
        get { return CheckHandRank(this);  }
    }
    public Hand(List<Card> cards)
    {
        if (cards.Count != 5)
        {
            throw new ArgumentOutOfRangeException("Invalid amount of Cards");
        }
        Cards = new List<Card>(cards);
    }
    public Hand()
    {
        this.Cards = Deck.Deal5Cards(deck);
    }
    public override string ToString()
    {
        sortHandbyFaceDesc(this);
        return string.Join(" ", Cards.Select(c => $"({c.Face} of {c.Suit}'s)"));     
    }
    private static int SortandCompareFaceValues(Hand hand, Hand secondhand)
    {
        sortHandbyFaceDesc(hand);
        sortHandbyFaceDesc(secondhand);
        for (int i = 4; 0 <= i; i--)
        {
            if (hand.Cards[i].Face == secondhand.Cards[i].Face)
            {
                if (i == 0) return 0;
                continue;
            }
            return hand.Cards[i].Face > secondhand.Cards[i].Face ? 1 : -1;
        }
        return 0; // should never get here but how to handle
    }
    public int CompareTo(Hand other)
    {
        if (HandRank == other.HandRank) //if the hand rank is equal, sort the cards by face value and compare the two biggest
        {
            if (HandRank.Equals(PokerHandsRank.Pair) || HandRank.Equals(PokerHandsRank.TwoPair) || HandRank.Equals(PokerHandsRank.ThreeOfKind) || HandRank.Equals(PokerHandsRank.FullHouse) || HandRank.Equals(PokerHandsRank.FourOfKind))
            {                    
                int pairValue = (ComparePairValue(this, other)); //check the values of the pairs and judge which is highest value
                return pairValue == 0  ? SortandCompareFaceValues(this, other) :  pairValue; //if pair is equal rank and highest value is equal check for highcards
            }
            return SortandCompareFaceValues(this, other);
        }
        return HandRank > other.HandRank ? 1 : -1;
    }
    private static int ComparePairValue(Hand hand, Hand secondHand)
    {
        Face face1 = FindHighestPairValuesFace(hand);
        Face face2 = FindHighestPairValuesFace(secondHand);

        return face1 == face2 ? 0 : face1 > face2 ? 1 : -1;
    }
    private static Face FindHighestPairValuesFace(Hand hand)
    {
        return (from card in hand.Cards
                orderby card.Face descending //sort by face value first
                group card by new { card.Face } into g
                let count = g.Count()
                orderby count descending // sort by what cards appear the most
                select new { g.Key.Face }.Face).First(); //first is highest value pair that appears the most
    }
    public static void sortHandbyFaceDesc(Hand hand)
    {
        var sortCards = (from cards in hand.Cards
                         orderby cards.Face descending
                         select cards).ToList();
        hand.Cards = sortCards;
    }
    public static void sortHandbyFace(Hand hand)
    {
        var sortCards = (from cards in hand.Cards
                         orderby cards.Face
                         select cards).ToList();
        hand.Cards = sortCards;
    }    

Pokerhandchecker is not included to save space, see part 2 if you wish to view them logic to find all possible hands then best hand

      private static Hand findBestHandInList(List<Hand> possibleHands)
    {
        // return  possibleHands.OrderByDescending(h => h.HandRank).First().Cards;
        Hand returnList = new Hand(possibleHands[0].Cards);
        for (int i = 0; i < possibleHands.Count - 1; i++)
        {
            if (returnList.CompareTo(possibleHands[i + 1]) == -1)
            {
                returnList = possibleHands[i + 1];
            }
        }
        return returnList;
    }
    private static void addPossibleHandFromSevenCards(List<Card> cards, List<Hand> possibleHands)
    {
        List<Card> tempCardList = new List<Card>();  // select first card not to be in the hand
        for (int firstCard = 0; firstCard < cards.Count; firstCard++)
        {
            for (int secondCard = firstCard + 1; secondCard < cards.Count; secondCard++) // select first card not to be in the hand
            {
                for (int i = 0; i < cards.Count; i++)
                {
                    if (i != firstCard && i != secondCard) // every card that is not the first or second will added to the hand
                    {
                        tempCardList.Add(cards[i]);
                    }
                }
                possibleHands.Add(new Hand(tempCardList));
                tempCardList.Clear();
            }
        }
    }

Main Program Code to run the console.

        Console.BackgroundColor = ConsoleColor.DarkGreen;
        Console.ForegroundColor = ConsoleColor.White;
        Console.Clear();
        Hand tableHand = new Hand();
        Console.WriteLine("Cards on the table are:");
        PrintCards(tableHand);


        TwoCardHand your2Cards = new TwoCardHand();
        TwoCardHand their2Cards = new TwoCardHand();
        Console.WriteLine();
        Console.WriteLine("Your two cards are:");
        PrintCards(your2Cards.Cards);
        Console.WriteLine("Their two cards are:");
        PrintCards(their2Cards.Cards);
        Console.WriteLine();
        Console.Read();
        List<Card> YourSevenHand = new List<Card>(your2Cards.Cards.Concat(tableHand.Cards));
        List<Card> TheirSevenHand = new List<Card>(their2Cards.Cards.Concat(tableHand.Cards));

        List<Hand> yourPossibleHands = new List<Hand>();
        List<Hand> theirPossibleHands = new List<Hand>();

        addPossibleHandFromSevenCards(YourSevenHand, yourPossibleHands);
        addPossibleHandFromSevenCards(TheirSevenHand, theirPossibleHands);

        Hand yourFinalHand = findBestHandInList(yourPossibleHands);               
        Hand theirFinalHand = findBestHandInList(theirPossibleHands);

        Console.WriteLine(yourFinalHand.CompareTo(theirFinalHand) == 1 ? "You win" : yourFinalHand.CompareTo(theirFinalHand) == 0 ? "Draw" : "They win");

        Console.WriteLine(yourFinalHand);
        Console.WriteLine("You have a {0}", yourFinalHand.HandRank);
        Console.WriteLine(theirFinalHand);
        Console.WriteLine("They have a {0}", theirFinalHand.HandRank);

Am I Making too many instances here? can this be made better?

This will most likely be my last post on this project as I think I have reached the limit of my ability to expand it further. I genuinely appreciate all and any suggestions people have for my code.

If you wish to run it for yourself. Please see the card class, enums and Check hand rank in part 1 and 2 I believe they should still work.

\$\endgroup\$
  • 1
    \$\begingroup\$ The shuffle is not uniform. HandCheck from prior is not accurate. Not all hands compare on two cards. \$\endgroup\$ – paparazzo Jan 23 '17 at 16:57
  • \$\begingroup\$ If you're concerned about the shuffle you could maybe try a Fisher-Yates shuffle instead? \$\endgroup\$ – Bethan Jan 23 '17 at 17:01
3
\$\begingroup\$

You can use the Enum class to obtain all of the values in an enum:

Face[] faceNames =
{
    Face.Ace, Face.Two, Face.Three, Face.Four, Face.Five, Face.Six, Face.Seven, Face.Eight,
    Face.Nine, Face.Ten, Face.Jack, Face.Queen, Face.King
};
Suit[] allSuits = {Suit.Diamond, Suit.Club, Suit.Heart, Suit.Spade};

Can become:

Array faceNames = Enum.GetValues(typeof(Face));
Array allSuits = Enum.GetValues(typeof(Suit));

Or with T[]:

Face[] faceNames = Enum.GetValues(typeof(Face)).Cast<Face>().ToArray();
Suit[] allSuits = Enum.GetValues(typeof(Suit)).Cast<Suit>().ToArray();

Your 2 methods for dealing cards:

public static List<Card> Deal5Cards(Deck deck)
{
    List<Card> newHand = new List<Card>();
    for (int i = 0; i < 5; i++)
    {
        Card temp = deck.Cards[0];
        newHand.Add(temp);
        deck.Cards.RemoveAt(0);
    }
    return newHand;
}

public static List<Card> Deal2Cards(Deck deck)
{
    List<Card> newHand = new List<Card>();
    for (int i = 0; i < 2; i++)
    {
        Card temp = deck.Cards[0];
        newHand.Add(temp);
        deck.Cards.RemoveAt(0);
    }
    return newHand;
}

Can be made into a single method:

public static List<Card> DealNCards(Deck deck, int count)
{
    List<Card> newHand = new List<Card>();
    for (int i = 0; i < count; i++)
    {
        Card temp = deck.Cards[0];
        newHand.Add(temp);
        deck.Cards.RemoveAt(0);
    }
    return newHand;
}

Your third loop in addPossibleHandFromSevenCards can be shorten with LINQ

for (int i = 0; i < cards.Count; i++)
{
    if (i != firstCard && i != secondCard) // every card that is not the first or second will added to the hand
    {
        tempCardList.Add(cards[i]);
    }
}

Like this:

tempCardList.AddRange(cards.Where((t, i) => i != firstCard && i != secondCard));

Your 2 methods for sorting can be shorten with lambda syntax:

public static void sortHandbyFaceDesc(Hand hand)
{
    var sortCards = (from cards in hand.Cards
        orderby cards.Face descending
        select cards).ToList();
    hand.Cards = sortCards;
}

public static void sortHandbyFace(Hand hand)
{
    var sortCards = (from cards in hand.Cards
        orderby cards.Face
        select cards).ToList();
    hand.Cards = sortCards;
}

Like this:

public static void sortHandbyFaceDesc(Hand hand)
{
    hand.Cards = hand.Cards.OrderByDescending(cards => cards.Face).ToList();
}

public static void sortHandbyFace(Hand hand)
{
    hand.Cards = hand.Cards.OrderBy(cards => cards.Face).ToList();
}

ComparePairValue can be made shorter and cleaner with CompareTo

return face1 == face2 ? 0 : face1 > face2 ? 1 : -1;

Like this:

return face1.CompareTo(face2);

FindHighestPairValuesFace can be made shorter with lambda syntax:

private static Face FindHighestPairValuesFace(Hand hand)
{
    return (from card in hand.Cards
        orderby card.Face descending
        //sort by face value first
        group card by new {card.Face}
        into g
        let count = g.Count()
        orderby count descending // sort by what cards appear the most
        select new {g.Key.Face}.Face).First(); //first is highest value pair that appears the most
}

Like this:

private static Face FindHighestPairValuesFace(Hand hand)
{
    return hand.Cards.OrderByDescending(card => card.Face)
        .GroupBy(card => new {card.Face})
        .Select(g => new {g, count = g.Count()})
        .OrderByDescending(t => t.count)
        .Select(t => new {t.g.Key.Face}.Face).First(); //first is highest value pair that appears the most
}

You can use expression body for your property here:

public PokerHandsRank HandRank
{
    get { return CheckHandRank(this); }
}

Like this:

public PokerHandsRank HandRank => CheckHandRank(this);
\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply again, The final point about removing the the private set (is something i recently readded) as otherwise I can't sort order properly even with the hand.Cards = hand.Cards.OrderByDescending(cards => cards.Face).ToList(); code you provided \$\endgroup\$ – Steve Jan 23 '17 at 16:16
  • \$\begingroup\$ I'm not sure what you mean, they are virtually equivalent. \$\endgroup\$ – Denis Jan 23 '17 at 16:29
  • \$\begingroup\$ It doesn't work anymore because you suggested it to be a readonly property by removing the private set on Cards. (both my old code and your new one) \$\endgroup\$ – Steve Jan 23 '17 at 16:35
  • 1
    \$\begingroup\$ @Steve I removed it from the answer, I also added LINQ version for your third loop in addPossibleHandFromSevenCards. \$\endgroup\$ – Denis Jan 23 '17 at 16:40
  • \$\begingroup\$ great thanks, works perfectly. I'll keep the question open a bit longer to see if there any other ideas/answers but I'll most likely mark this as the answer later \$\endgroup\$ – Steve Jan 23 '17 at 16:53

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.