6
\$\begingroup\$

For practicing/learning object orientation, I made a Card class and am practicing doing stuff with it such as shuffling decks and dealing hands etc...

I have a method that receives a list item which essentially is a hand of 5 cards then tells the user how about the hand such as pairs and three of a kind however it feels incredibly sloppy and would like to know if it's improvable.

private static string CheckHand(List<Card> hand)
{
    string[] tempnames = { hand[0].Face, hand[1].Face, hand[2].Face, hand[3].Face, hand[4].Face };         
    int sameCardCount = 0;     

    for (int i = 0; i < hand.Count; i++)
    {
        for (int k = 0; k < hand.Count; k++)
        {
            if (k == i) { }
            else if (tempnames[i] == hand[k].Face)
            {                                       
               sameCardCount++;
            }
        }
    }
    switch (sameCardCount)
    {
        case 12:
        return "you have four of a kind!";
        case 6:
        return "you have three of a kind!";
        case 4:                  
        return "you have two pairs!";  
        case 2:   
        return "you have a pair!";
        default:
        return "nothing special here..";
    }
}

The card object and enum suit is like so:

class Card
{
    public string Face { get; set; }
    public Suit Suitc { get; set; }
    public Card(Suit suit,string face)
    {
        Face = face;
        Suitc = suit;
    }

    public override string ToString()
    {
        string card = "(" + Face + " " + Suitc + ")";
        return card;
    }
}
enum Suit
{
    CLUB, DIAMOND, HEART, SPADE
}

As far as I can test, it works as intended. But with the double loop, the return values are a bit messy in my opinion. How would you have done it?

Note: you do not have to provide extracts of code if you don't wish to. I'm just trying to help myself improve my problem solving.

\$\endgroup\$
  • 1
    \$\begingroup\$ You can try expressing what those "3 of a kind" mean via classes as a next exercise, so that PokerHandValue(['7♥', '3♥', '4♥', '5♥', '6♥']) > PokerHandValue(['2♥', '3♥', '4♥', '5♥', '7♣']). I've tried something similar here, although it's in Python and uses some magic. \$\endgroup\$ – Daerdemandt Jan 13 '17 at 19:16
  • 1
    \$\begingroup\$ Many classifiers choose to store cards as a bitfield. You only need (13+4) bits. It's a very compact representation, plus there are lots of tricks for fast hand classification -- for example if you "and" all the cards in a hand you can check the suite bits (and'ing with a bitmask, and comparing to zero) to detect a flush. \$\endgroup\$ – Chuu Jan 13 '17 at 19:39
6
\$\begingroup\$

Improving your Card class

I can see that your code is not working for all the different combinations, so I wont comment on that, but there are other things you might improve :

Enums

Face should be an enum as well :

public enum Face
{
    Two,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
    Ten,
    Jack,
    Queen,
    King,
    Ace
}

Your Suit property should be called simply Suit

Interpolated string

Your .ToString() can use interpolated string :

public override string ToString()
{
    return $"({Face} {Suit})";
}

The class as a whole

The Card can't mutate/change during it's lifetime which is also know in programming as immutable object.

This is object that can receive values only upon instantiation, if you want to change some value, you need to create new object, an example of immutable object is string.

So how do you achieve immutability in C# ? With the help of properties backed with readonly field. Readonly allows a value to be assigned to an object only at instantiation, or in the constructor. C# 6 offers us implicit creating of readonly properties which are also know as get only properties.

public Face Face { get; }
public Suit Suit { get; }

With all of that your card class should look like this :

public class Card
{
    public Face Face { get; }
    public Suit Suit { get; }

    public Card(Suit suit, Face face)
    {
        Face = face;
        Suit = suit;
    }

    public override string ToString()
    {
        return $"({Face} {Suit})";
    }
}

Improving your CheckHand(List<Card> hand) method

There are somethings to improve in your for loop:

for (int i = 0; i < hand.Count; i++)
{
    for (int k = 0; k < hand.Count; k++)
    {
        if (k == i)
        {

        }
        else if (tempnames[i] == hand[k].Face)
        {
            sameCardCount++;
        }
    }
}

The empty if (k == i) shouldn't be there at all instead, negate it and merge the else if into 1 if like this:

if (k != i && tempnames[i] == hand[k].Face)
{
    sameCardCount++;
}

You can also apply some LINQ to your inner loop and transform the for loop to this:

for (int i = 0; i < hand.Count; i++)
{
    sameCardCount += hand.Where((card, j) => j != i && tempnames[i] == card.Face).Count();
}

Where card is the current card and j is the index.

You can also transform your whole loop into single LINQ expression:

int sameCardCount = hand
    .Select((card, i) => hand
    .Where((card, j) => j != i && tempnames[i] == card.Face)
    .Count())
    .Sum();

Where card is the current card and i is the index.

You should also check if the length of the parameter hand is equal to 5, else your program will crash add on top of the method like this:

if (hand.Count != 5)
{
    throw new ArgumentException("Hand must have length of 5.");
}

Even tho I think you should have a designated class for Hand and accept that as parameter.

Code style

MAGIC NUMBERS

The term magic number is referring to numbers that the reader of your code (not you) wont be able to tell where they are coming from, you have a bunch of those in your switch case:

switch (sameCardCount)
{
    case 12:
        return "you have four of a kind!";
    case 6:
        return "you have three of a kind!";
    case 4:
        return "you have two pairs!";
    case 2:
        return "you have a pair!";
    default:
        return "nothing special here..";
}

2, 4, 6, 12 they are all magic numbers. You should declare them as constant variables with descriptive names:

private const int pair = 2;
private const int twoPair = 4;
private const int threeOfAKind = 6;
private const int fourOfAKind = 12;

And your switch case can look like this:

switch (sameCardCount)
{
    case fourOfAKind:
        return "you have four of a kind!";
    case threeOfAKind:
        return "you have three of a kind!";
    case twoPair:
        return "you have two pairs!";
    case pair:
        return "you have a pair!";
    default:
        return "nothing special here..";
}

If you are planning to expand your program, I recommend posting your code for review here every now and than, as I can tell you are still beginner level and getting reviews here might be extremely useful for you, to teach you good practices, show you new ways of doing stuff and basically improve your code.

Good luck!

\$\endgroup\$
  • \$\begingroup\$ The first change in the "Improving your CheckHand(...) method" should be if (k != i && tempnames[i] == hand[k].Face). \$\endgroup\$ – Mark H Jan 13 '17 at 23:35
  • \$\begingroup\$ It's the first thing written under that section. \$\endgroup\$ – Denis Jan 13 '17 at 23:41
  • \$\begingroup\$ You have k == i. \$\endgroup\$ – Mark H Jan 14 '17 at 1:04
  • \$\begingroup\$ Ops thanks for that, I edited the answer my bad :) \$\endgroup\$ – Denis Jan 14 '17 at 1:08
  • \$\begingroup\$ Wow wasn't expecting this much detail when i posted here! loads of stuff i never considered, made me really excited to try these new things. Thanks a lot :) \$\endgroup\$ – Steve Jan 16 '17 at 9:12
7
\$\begingroup\$

Much of your code can be made shorter and more clear with LINQ:

string[] tempnames = { hand[0].Face, hand[1].Face, hand[2].Face, hand[3].Face, hand[4].Face };         

vs

var faces = hand.Select(c => c.Face).ToArray();

But we actually don't need this at all, because your main algorithm can be made more clear with LINQ as well. What do you want? To divide the cards up into groups based on their face value. So do that!

var cardCounts = (from card in hand         // We're processing each card
                 group card by card.Face    // make a sequence of groups of cards with the same face
                 into g                     // process each group 
                 let count = g.Count()      // get the size of each group
                 orderby descending count   // sort the groups by size, big to small
                 select count).Take(2).ToList();  // get the top two sizes

Before you read on: Why did I only take the highest two counts?

And now you can just read off the results:

switch(cardCounts[0]) 
{
  case 1: return "nothing";
  case 2: return cardCounts[1] == 1 ? "one pair" : "two pair";
  case 3: return cardCounts[1] == 1 ? "three of a kind" : "full house";
  case 4: return "four of a kind";
  default: Debug.Fail("How'd we get here?"); return "";
}

LINQ make the code read more like its meaning rather than a bunch of loops and counters that seem completely disconnected from the domain at hand. What are you doing logically? Grouping the cards by face value and counting how many cards are in each group. So make the code actually do that.

Remember, your goal here is to correctly implement the rules. The rules do not say "consider all ordered pairs of cards; assign one point for each identical card in that set of pairs. If the point total is 12 then..." and so on, but that's what you wrote. Which is more likely to be correct, code that resembles the logic of the rules, or code that does some completely other thing that you hope is equivalent to the rules? Which do you suppose will be easier to maintain?

Exercise: Now add flushes, straights, and straight flushes. Can you use LINQ?

Exercise: Come up with a better representation for the value of a hand other than a string describing it

Exercise: Make a class for hands. What operations do you need to perform on hands?

Exercise: Cards don't change their kind, but you've modeled them as having setters. Make the card type an immutable struct, not a mutable class.

Exercise: Implement IComparable<Card> on cards.

Exercise: Given two hands, which, if either, is the winning hand? Implement IComparable<Hand> on Hand. Note that you'll need more than just the kind of hand it is! Suppose there are two hands, both are two pair, and both are pair of kings, pair of queens. One of them still could win. Read the rules of poker carefully; that's your specification. Again, try to make the code read like the rules.

Exercise: Given a sequence of cards, generate all possible five-card hands. The signature of your method should be static IEnumerable<Hand> AllHands(IEnumerable<Card> cards). If there are fewer than 5 cards in the sequence then clearly the resulting sequence should be empty. If there are exactly 5 then you should get exactly one hand. If there are 6 cards then you should get 5 hands. If there are 7 cards then how many hands should you get?

Exercise: given a sequence of 7 cards, find the highest 5-card hand in it.

Exercise: given n two-card hands and five community cards, describe how to divide the pot. Note that you have to represent ties somehow. Again, remember, you are implementing the rules of poker, and those include ties.

Exercise: What if there are multiple pots, due to an all-in situation? Now how to divide the pots?

Exercise: Implement the rest of the rules of poker: ante, betting, raising, etc.

Exercise: Write an AI that plays poker, and a user interface that lets humans play against the AI.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your answer, given me loads to think about. as you guessed i planned to check the hands for same suits as well but same face checker was much harder than i thought. I had planned to practice linq a lot more so I'll give your exercises a try. thanks again really appropriate it \$\endgroup\$ – Steve Jan 16 '17 at 9:17
  • \$\begingroup\$ [link]codereview.stackexchange.com/questions/152857/… I've started doing some of your exercises and I would love to know what you think if you get time. plus I feel i owe you an accepted answer for the effort you put in. I'm still working on the final 5/6 points though. \$\endgroup\$ – Steve Jan 18 '17 at 10:57
3
\$\begingroup\$

Bearing in mind I know nothing about cards so these terms don't make sense to me...

private static string CheckHand(List<Card> hand)

"CheckHand" doesn't tell me what it checks or what kind of data would be returned in the string.

A "hand" sounds like an object (maybe not a physical object, maybe it's conceptual, but that's still an object). If you can ask questions about a "hand", or perform actions on a "hand", or get information from a "hand", then it should be its own object type.

for (int i = 0; i < hand.Count; i++)
{
    for (int k = 0; k < hand.Count; k++)
    {
        if (k == i) { }
        else if (tempnames[i] == hand[k].Face)
        {                                       
           sameCardCount++;
        }
    }
}

This should be extracted into its own method, and given a name that says what it calculates.

switch (sameCardCount)
{
    case 12:
    return "you have four of a kind!";
    case 6:
    return "you have three of a kind!";
    case 4:                  
    return "you have two pairs!";  
    case 2:   
    return "you have a pair!";
    default:
    return "nothing special here..";
}

The magic numbers should become well-named constants. And the strings could become values of an enum that contains all possible results. That will allow you to return an enum e.g. Result.TwoPairs, so you don't have to parse the string to figure out what the result was, and it also allows the caller to then decide how to display the information.

class Card
{
    public string Face { get; set; }
    public Suit Suitc { get; set; }

I googled what a card "face" was, and it sounds more like a bool than a string (a card is or isn't a face, right?). No need to call your suit "Suitc", you can call it "Suit".

Also depending on how you use the Suit information, it might be a better idea to subclass the card types, rather than use an enum. For example if there is card behaviour or attributes that would depend on Suit, each subclass can override a base method and implement Suit-specific behaviour, without the calling code needing to know and implement the Suit-based logic.

public override string ToString()
{
    string card = "(" + Face + " " + Suitc + ")";
    return card;
}

You can return the string without assigning to a variable, i.e. return "(" + Face + " " + Suitc + ")";

enum Suit
{
    CLUB, DIAMOND, HEART, SPADE
}

Enums should be PascalCase.

\$\endgroup\$
  • 5
    \$\begingroup\$ I am a poker player and hand is a common term. The number is typically called rank but the problem is full house, quads ... is also called rank so I think face or number is a good name. \$\endgroup\$ – paparazzo Jan 13 '17 at 17:40
0
\$\begingroup\$

Check length of input

Why string[] tempnames
Just use hand[i].Face == hand[k].Face

You don't check for full house, straight or flush

If you are going to have an enum for suit you should have an enum for face

That loop is not terribly efficient but it seems to work for Face

Have a constructor that takes 0-51 and converts with i%13 i/13

Also 5 cards is only useful for 5 card draw. For anything with board you also need to determine the best 5 cards which is more complex.

Hand should be a Class - more self contained
For example compare two hands is very important

public enum Faces { Two, Three };
public enum Suits { Heart, Spade };
public enum Ranks { HighCard, OnePair, TwoPair };
public class Card : Object
{
    public Faces Face { get; set; }
    public Suits Suit { get; set; }
    public Card (byte b)
    {
        Face = (Faces)(b % 13);
        Suit = (Suits)(b / 13);
    }
    // need implement Equals so you can compare cards
}
public class Hand : IComparable
{
    public List<Card> Cards { get; set; }
    public Ranks? Rank
    {
        get
        {
            if (Cards.Count != 5)
            {
                return null;
            }
            int count = 0;
            for(int i = 0; i < Cards.Count - 1; i++)
            {
                for(int j = i+1; j < Cards.Count; j++)
                {
                    if(Cards[i].Face == Cards[j].Face)
                    {
                        count++;
                    }
                }
            }
            // recalibrate the switch
            return Ranks.HighCard;
        }
    }  // use CheckHand() here
    public int CompareTo(object obj)
    {
        if (Rank == null)
        {
            throw new ArgumentNullException();
        }
        if (obj == null)
        {
            throw new ArgumentNullException();
        }
        if (obj is Hand)
        {
            Hand otherHand = (Hand)obj;
            if (otherHand.Rank == null)
            {
                throw new ArgumentNullException();
            }
            return ((int)this.Rank).CompareTo((int)otherHand.Rank);
            // this is really more complex as Kings Up beats Tens Up  
            // how I do this is an intger for stength
        }
        else
        {
            throw new ArgumentNullException();
        }
    }
}
\$\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.