10
\$\begingroup\$

This represents a table (server) for Texas Hold'em.

This is not a full Hold'em game but it is working code for what it does. It shuffles, deals the hole cards, deals the board, moves the blinds, and collects the blinds. It does not yet support real betting nor award the pot. It just fakes a pot. Real betting gets complex. It is at a point I would appreciate a review.

In Poker the button moves to the dealers right which is the players left (+1) (clockwise). For the first round there is a random draw for the button. There are 4 rounds of betting/dealing. The small blind is left of the button (+1) has a forced bet of SB. The big blind to the left of SB has a forced bet of BB (typically 2x the SB). After the BB is first to act in the first betting round. After the first betting round the SB is first to act. In the first street (pre) each player gets two hole cards. On the second street (flop) the board gets three cards. Turn and river the board gets one more card each. I know it sounds complex but that is how it works.

In heads up (two players) it gets messy as the dealer is also the SB. This does not yet deal with heads up.

A table has seats. This code does not use seat. The ordered List of players is the seats. This code moves the button directly to a player and assigns betting order directly to a player.

There is no dealer in this code. The table performs the actions of the dealer.

This uses a standard 52 card poker deck and no intention of ever supporting another deck.

This does not burn cards which has no effect on the game.

Not intended to be gaming level server where money is exchanged. Just host some simple home games.

public static void PokerPlayerTest()
{
    PokerTable pt = new PokerTable("Hook", new List<PokerPlayer> { new PokerPlayer("Peter0"), new PokerPlayer("Tinker1"), new PokerPlayer("Wendy2"), new PokerPlayer("Rianna3"), new PokerPlayer("Lady4") });
    for(int i = 0; i<10; i++)
    {
        pt.Shuffle();
        Debug.WriteLine(pt.ToString());
        pt.Flop();
        Debug.WriteLine(pt.ToString());
        pt.Trun();
        Debug.WriteLine(pt.ToString());
        pt.River();
        Debug.WriteLine(pt.ToString());
        Debug.WriteLine("");               
    }
}
public class PokerCard
{
    byte id;
    public enum Suits { heart, spade, diamond, club };
    public enum Ranks { two, three, four, five, six, seven, eight, nine, ten, jack, queen, king, ace };
    public Suits Suit { get { return (Suits)(id/13); } }
    public Ranks Rank { get { return (Ranks)(id%13); } }
    public override string ToString()
    {
        return $"{Rank} {Suit}";
    }
    public PokerCard(byte ID)
    {
        id = ID;
    }
}
public class PokerPlayer
{
    public enum Poss { sb, bb, btn, mid}
    //current PokerPlayer does not bet
    public int ChipCount { get; set; } = 200;
    public PokerCard[] HoleCards { get; } = new PokerCard[2];
    public int ID { get; }
    public int BettingOrder { get; set; } = 0; 
    public Poss Pos { get; set; } 
    public string Name { get; }
    public override string ToString()
    {
        return $"{Name} {HoleCards[0].ToString()}_{HoleCards[1].ToString()}  pos {Pos}  chips {ChipCount.ToString("N0")}";
    }
    public PokerPlayer (byte id, string name)
    {
        ID = id;
        Name = name;
    }
    public PokerPlayer(byte id, string name, int chipCount)
    {
        ID = id;
        Name = name;
        ChipCount = chipCount;
    }
    public PokerPlayer(string name)
    {
        ID = 0;
        Name = name;
    }
    public PokerPlayer(string name, int chipCount)
    {
        ID = 0;
        Name = name;
        ChipCount = chipCount;
    }
}
public class PokerTable
{
    //pot and betting not yet implemented 
    //current PokerTable just deals
    Random rand = new Random();
    List<PokerCard> deck = new List<PokerCard>();
    public enum Streets { pre, flop, turn, rivr }
    byte street = 0;
    byte cardNum = 0;
    int round = 0;
    int btn = 0;
    int sb = 1;
    int bb = 2;
    public PokerPlayer Button {  get { return PokerPlayers[btn]; } }
    public int Pot { get; set; } = 0;
    public Streets Street { get { return (Streets)street; } }
    public List<PokerCard> Board { get; } = new List<PokerCard>();
    public string BoardStr { get { return string.Join(", ", Board.Select(c => c.ToString())); } }
    public string PlayersStr { get { return string.Join(", ", PokerPlayers.OrderBy(p => p.BettingOrder).Select(p => p.ToString())); } }
    void SetBettingOrder()
    {
        int cnt = PokerPlayers.Count;
        int offset = street == 0 ? 3 : 1;
        for(int i = 0; i < PokerPlayers.Count; i++)
        {
            int bettingOrder = (i + 2*cnt - btn - offset) % cnt;
            PokerPlayers[i].BettingOrder = bettingOrder;
        }
    }
    public override string ToString()
    {
        StringBuilder sb = new StringBuilder();
        if(street == 0)
        {
            sb.AppendLine($"dealer {DealerName}  round {round}  button {Button.Name}\n  {Street} "); //{ PlayersStr}
        }
        else 
        {
            sb.AppendLine($"  {Street} {BoardStr}");
        }
        foreach (PokerPlayer pp in PokerPlayers.OrderBy(p => p.BettingOrder))
        {
            sb.AppendLine($"       {pp.ToString()}");
        }
        sb.Append($"       pot {Pot}");
        return sb.ToString();
    }
    public string DealerName { get; }
    public List<PokerPlayer> PokerPlayers { get; } 
    private void MoveButton()
    {
        if (round == 1)
        {                   
            btn = rand.Next(PokerPlayers.Count - 1);               
        }
        else
        {
            btn++;
            if(btn == PokerPlayers.Count)
            {
                btn = 0;
            }
        }
        foreach (PokerPlayer pp in PokerPlayers)
        {
            pp.Pos = PokerPlayer.Poss.mid;
        }              
        int tpos = btn + 1;
        if(tpos == PokerPlayers.Count)
        {
            tpos = 0;
        }
        PokerPlayers[tpos].Pos = PokerPlayer.Poss.sb;
        PokerPlayers[tpos].ChipCount -= sb;
        tpos ++;
        if (tpos == PokerPlayers.Count)
        {
            tpos = 0;
        }
        PokerPlayers[tpos].Pos = PokerPlayer.Poss.bb;
        PokerPlayers[tpos].ChipCount -= bb;
        PokerPlayers[btn].Pos = PokerPlayer.Poss.btn;
    }
    public void Shuffle()
    {
        //fisher yates shuffle
        street = 0;              
        round++;
        Pot = sb + bb;
        Board.Clear();
        MoveButton();
        SetBettingOrder();
        for (int b = deck.Count - 1; b > 0; b--)
        {
            int r = rand.Next(b);
            if(r != b)
            {
                PokerCard temp = deck[r];
                deck[r] = deck[b];
                deck[b] = temp;
            }
        }
        //no need for cut or burn cards cards
        cardNum = 0;
        foreach(PokerPlayer pp in PokerPlayers)
        {
            pp.HoleCards[0] = deck[cardNum];
            cardNum++;
            pp.HoleCards[1] = deck[cardNum];
            cardNum++;
        }
    }
    public void Flop()
    {
        if(street != 0)
        {
            throw new ArgumentOutOfRangeException("wrong street");
        }
        street++;
        SetBettingOrder();
        Board.Add(deck[cardNum]);
        cardNum++;
        Board.Add(deck[cardNum]);
        cardNum++;
        Board.Add(deck[cardNum]);
        cardNum++;
        Pot += Pot;
    }
    public void Trun()
    {
        if (street != 1)
        {
            throw new ArgumentOutOfRangeException("wrong street");
        }
        street++;
        Board.Add(deck[cardNum]);
        cardNum++;
        Pot += Pot;
    }
    public void River()
    {
        if (street != 2)
        {
            throw new ArgumentOutOfRangeException("wrong street");
        }
        street++;
        Board.Add(deck[cardNum]);
        Pot += Pot;
    }
    public PokerTable(string dealerName, List<PokerPlayer> pokerPlayers)
    {
        DealerName = dealerName;
        PokerPlayers = pokerPlayers;
        for(byte b = 0; b < 52; b++)
        {
            deck.Add(new PokerCard(b));
        }
    }
}

Getting a few (disparaging) comments about short enum. When you are fighting for screen space they are nice. Poker has some standard abbreviations. Client is free to use any text or image they want (and should). table positions

Can ignore this
This is for for a service oriented architecture (SAO). To add that would possibly invalidate existing answers which is not fair.

\$\endgroup\$
  • \$\begingroup\$ Something is off with the braces / indent, ..or is the public static void PokerPlayerTest() scope just... floating in the namespace? \$\endgroup\$ – Mathieu Guindon Mar 19 '18 at 1:39
  • \$\begingroup\$ @MathieuGuindon It is not showing the namespace. \$\endgroup\$ – paparazzo Mar 19 '18 at 4:29
  • \$\begingroup\$ I know. What I'm saying is that the method isn't in any class. \$\endgroup\$ – Mathieu Guindon Mar 19 '18 at 4:30
  • 1
    \$\begingroup\$ @MathieuGuindon Correct. It is just a test method. What do you want me to do? \$\endgroup\$ – paparazzo Mar 19 '18 at 4:32
  • \$\begingroup\$ In Poker the button moves to the right (+1). From a player's perspective, it moves to the left, not the right. Are you thinking in a top-down view, i.e. where the button moves from 12 o'clock to 1 o'clock? Because that would be a confusing definition: 12 to 1 move to the right, but 6 to 7 moves to the left. \$\endgroup\$ – Flater Mar 19 '18 at 11:44
7
\$\begingroup\$

I'm going to second downrep_nation's comments: sb, bb, rivr, etc. are terrible names for an enum elements (and members). I'd much prefer smallBlind bigBlind, and button: btn looks like a control from the past. I use initials for types all the time, but never for domain concepts, and what good will removing the 'e' from river do anyone?

This is also a terrible identifier: Poss. PokerTable.Trun has a spelling error.


Speaking of the Trun, why are you not using your Streets enum?

if (street != 1)
{
    throw new ArgumentOutOfRangeException("wrong street");
}
street++

Could be

if (street != Street.Flop)
{
    throw new ArgumentOutOfRangeException("Wrong street: expected Flop, but received " + street.ToString());
}
street = Street.Turn;

Which is much clearer than depending on the detail of enum orderings (which is not documented at all). This, of course, depends on street being a Streets, not a byte, which would also be much clearer.

In SetBettingOrder:

int offset = street == Steets.Pre ? 3 : 1;

There is an awful lot of 'add one possibly reset' in your code, which heavily breaks up the logic, and is just more stuff to worry about during maintaince:

int tpos = btn + 1;
if(tpos == PokerPlayers.Count)
{
    tpos = 0;
}

A much shorter way is tpos = (tpos + 1) % PokerPlayers.Count, but given how often you need this, I would definitely make this it's own function:

private int NextPlayerIndex(int playerIndex)
{
    return (playerIndex + 1) % PokerPlayers.Count;
}

You also have a great many constructors... this one makes no sense:

public PokerPlayer(string name){
    ID = 0;
    Name = name;
}

In what world would not having to specify the ID be a good thing? This is just begging to be misused.

Given there is no logic in your constructors, I'd also prefer to see one 'base' constructor, which the others (though honestly I'd remove all but one) call into, rather than each maintaining it's own list of defaults:

public PokerPlayer(byte id, string name, int chipCount)
{
    ID = id;
    Name = name;
    ChipCount = chipCount;
}

public PokerPlayer (byte id, string name) : this(id, name, 200)
{
    // nix // I usually leave a short, semi-esoteric but recognisable comment to indicate there is meant to be nothing here when I have a pair of empty braces
}

This means that if you add a member, you only need update the base constructor and fix all the errors that produces, rather than worrying about not missing any. In your particular case, chipCount is a bit weird, because it has a common and unusual default, but I'm assuming that is a relic of debugging, and not something that would ever appear in production code: if such a magic number did appear, it should be a constant which you can reference in your constructors.

`const int DefaultChipCount = 200`

Alternatively, you can use an optional parameter, and ditch all of the horridness at the loss of some flexibility.

public PokerPlayer(byte id, string name, int chipCount = DefaultChipCount)

This means you can't set a default for id in its present position, but if it was so unimportant it could have a default, it wouldn't be the first parameter.


Again as downrep_nation said, there is a lot of confusion about what some of your methods do. Shuffling an array (as performed in Shuffle) in particular has nothing to do with Poker, and I would implement this a completely separate class because it is inherently a general-purpose and generic operation. Indeed, I would guess from the position of the comment //fisher yates shuffle that originally Shuffle was just going to shuffle, but then became inflated with other duties.


I'll take downrep_nation's advise about this code in a slightly different direction:

pp.HoleCards[0] = deck[cardNum];
cardNum++;

This should be pushed into a TakeCard() => cardNum++; method. Assembling the community cards then gives you the following, which I think is much more expressive than downrep_nation's LINQ:

for (int i = 0; i < magicNumber; i++)
    Board.Add(TakeCard);

The pokerPlayers parameter here is kind of scary, because you don't know where it is coming from, and you then proceed to use it for state:

PokerTable(string dealerName, List<PokerPlayer> pokerPlayers)

I would make it (and the property, especially so because it's public) an IReadOnlyList<PokerPlayer>, and clone it, just to be sure. You do not want the code that created your PokerTable unilaterally changing this list.


And a few misc/boring things:

  • Just personal preference really, but I would be filling your code with empty lines to break it up a but. It is so much easier to navigate 'segmented' code: it's the same reason we use bullet points and horizontal rules in CR answers

  • I'm not overly fond of using enums to give things names: suit and rank are both public despite being useless outside of PokerCard and having private-looking names, and provide no protection against an 'out of bounds' lookup. PokerCard looks like a great place to have a few Debug.Assert statements, so that if it is ever provided with an invalid id it can throw immediately, giving you a valuable call-stack, and providing some documentation.

  • I also specify the visibility of all members (e.g. PokerCard.id): but obviously this isn't a massive concern (unless you are working with someone coming from C++ maybe/any other language with different defaults)

  • I would be inclined to tear out Poss Pos in PokerPlayer, and just compute it when it is needed, unless the PokerPlayer class has a good reason to need to know its position.

  • I'm sure they are just for debugging, but the ToString() methods don't make a lot of sense. PokerPlayer can return a somewhat-uninitialized state when ToString() is called (Pos will be a default value).

  • In PokerTable, DealerName and PokerPlayers are defined below some methods, which will confuse anyone looking for them.

  • There is nothing stopping a player from going into a negative chip count (this is all part of what you've noticed that you have only players, and not seats, so you can't evict players who have run out of funds).

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot of great stuff. I like sb, bb, and btn for now as they make sense to me. This is a server. I like NextPlayerIndex. CTORS have already been cleaned up. Agree on IReadOnlyList<PokerPlayer> for now it is just me on the client and hack around a bit - will lock it down later. Poker Player has very good reason to know position (Poss) - rules, strategy and analytics need that on both the server and client. As stated betting is not yet active. ChipCount is just a place holder for now. A busted player will just not active but betting is not yet active. \$\endgroup\$ – paparazzo Mar 19 '18 at 10:26
  • \$\begingroup\$ Don't need seats to evict players. For hosting variable table size for now I like this model of List. Deal with empty seats is logic. If I was going to have a multi-table tournament then I would probably go with seats but that is not my goal. Thanks again really good feedback. \$\endgroup\$ – paparazzo Mar 19 '18 at 10:34
  • \$\begingroup\$ Oh ID means nothing right now. Eventually that will be the PK of the player from the database. \$\endgroup\$ – paparazzo Mar 19 '18 at 10:35
  • \$\begingroup\$ "Wrong street: expected Flop, because saw " + street.ToString() isn't really the best error message either. "Because saw" doesn't make semantical sense here. As an improved suggestion: "Wrong street: expected Flop, but received " + street.ToString() \$\endgroup\$ – Flater Mar 19 '18 at 13:34
  • \$\begingroup\$ @Flater of course I meant "but"... thanks (always been terrible at proof-reading) \$\endgroup\$ – VisualMelon Mar 19 '18 at 13:35
10
\$\begingroup\$

Overall the code seems ok.

But there are some things to consider which have to do with cleaner code and SOLID principles.

Firstly your variable names are confusing and not indicative.

sb, bb, btn, cardNum

Mean nothing to me, maybe they're poker terms but I'm not a poker player, I'm a developer and I'm reading your code.

There is inconsistency with id being once

ID and id

Chose a casing style and stay with it

public Suits Suit { get { return (Suits)(id/13); } }

This can be written as

public Suits Suit => (Suits)(id/13);

Which is just syntactic sugar for getter only members but is not a must, what is a must on the other hand is naming your magic numbers

I don't know what 13 is supposed to represent, consider creating a constant to store this value for better understandability, this repeats itself all around the code.

In the PokerPlayer class consider using default arguments for the function parameters instead to remove some code repetition.

Another point would be SOLID principles, especially SRP (single responsibility)

Your functions break SRP by doing a lot of things, combining both graph logic and business logic, meaning a function should or aggregate different abstractions to define logic, or be responsible for lower level workings with code and its underlying data structures.

Additionally SRP makes functions more indicative and testable Shuffle() being a good example by both implementing low level array shuffling and also dealing with abstractions like Board.Clear().

Try to divide those functions into logical components and write tests for them

In Flop() you could write Pot *= 2 instead to indicate doubling of the pot (nitpick)

Additionally this could be replaced with easier to read Linq

Board.Add(deck[cardNum]);
cardNum++;
Board.Add(deck[cardNum]);
cardNum++;
Board.Add(deck[cardNum]);
cardNum++;

Instead write

Board.AddRange(deck.Skip(cardNum).Take(magicNumber));
cardNum+=magicNumber;
\$\endgroup\$
  • \$\begingroup\$ Thanks. sb bb and btn are standard poker terms. cardNum is just used by the program to keep track where it is in the deck. 13 is a magic number that will not change but I can make it a constant. \$\endgroup\$ – paparazzo Mar 19 '18 at 6:36
  • \$\begingroup\$ @paparazzo Are sb, bb, and btn small blind, big blind, and button? If so, is there any real downside to naming them smallBlind, bigBlind and button? Those seem much easier to read. At the very least, you should get rid of the variable conflict between sb at the class-level, and the StringBuilder in your ToString method. \$\endgroup\$ – Kris Harper Mar 19 '18 at 14:37
  • \$\begingroup\$ @KrisHarper Yes they are. One of the outputs is hand history that wants small. On a phone I want small. I will add longer descriptions to the enum and the client will have the choice of which to use. The client can adjust. The client will typically use a dealer button and none of the enum values. \$\endgroup\$ – paparazzo Mar 19 '18 at 14:48
  • 3
    \$\begingroup\$ @paparazzo I saw your edit. I don't think the name recommendations are disparaging. You are asking for a code review, and a lot of people are telling you that your variable names are too short because that is an extremely common piece of feedback in code reviews. It is not just that they are obscure, it's also that using two-letter variable names drastically reduces your namespace. You already have a name conflict in your ~100-line program. I honestly don't really care what you call them, but I think it's silly to ask for advice, and then completely ignore consistent feedback. \$\endgroup\$ – Kris Harper Mar 19 '18 at 18:16
  • 2
    \$\begingroup\$ @paparazzo: More generally: the implementation details of your program and the construction of the user interface should have nothing to do with each other. Name the parts of your program what makes sense as parts of a C# program using standard C# conventions. If you need to translate those program element values into human-readable text then write a function that does that. \$\endgroup\$ – Eric Lippert Mar 19 '18 at 21:22
7
\$\begingroup\$

General comments

  • Smurf naming convention: PokerCard, PokerPlayer, PokerTable, PokerPlayerTest can easily be renamed to Card, Player, Table, PlayerTest without any drawbacks.
  • In general, you abbreviate names much more than you should. It makes the code less obvious to follow.
    • You shouldn't abbreviate class/enum definitions.
    • It's more acceptable to abbreviate the names of variables (within reason).
    • Position playerPosition; is good. Position playerPos; is acceptable. Poss playerPosition; or Poss playerPoss; are not good.
  • A player with 1000 chips in hand can choose to only place 250 chips on a particular poker table. Your current implementation assumes that a player automatically places all their chipes on a table. However, I'm going to assume that this is simply out of scope for your current application.
    • I do think that it'd be a good next exercise for you to learn how to separate data that belongs to a player, and data that belongs to a player's seat on a table. This is the biggest "mistake" (mind the quotes) that you've made in your current approach.

PokerCard

While I agree with the enum for both card suit and card value, I disagree with some other things.

  • Why are you using an ID of type byte? byte is not commonly used for data models that don't rely on binary data.
  • Instead of using an ID to then calculcate the suit and value, simply set the suit/value directly and remove the ID.
    • Your current implementation relies on the exact amount of suits and values that currently exist. I'm aware that playing cards aren't expected to add/remove suits/values anytime soon, but it's the principle of the thing. Don't assume that the amount of values in an enum is never going to change.
  • I would rename Rank to Value. Rank implies that they are sorted in order, but the Ace simultaneuously functions as a "King+1" but also as a "one" (i.e. Ace-2-3-4-5 is a valid straight). Therefore, the Ace is not ranked correctly, so it's technically not a card rank.
  • Minor comment: Card suits are plural: hearts, spades, diamonds, clubs.
  • Minor comment: I changed the ToString() to reflect how English refers to playing cards, e.g. "two of spades".

Revised class:

public class PokerCard
{
    public enum Suits { hearts, spades, diamonds, clubs };
    public enum Values { two, three, four, five, six, seven, eight, nine, ten, jack, queen, king, ace };
    public Suits Suit { get; set; }
    public Values Value { get; set; }
    public override string ToString()
    {
        return $"{Value} of {Suit}";
    }

    public PokerCard(Suits suit, Values value)
    {
        this.Suit = suit;
        this.Value = value;
    }
}

PokerPlayer

  • The constructor takes a byte id but the class has a property int ID. Pick a type and stick to it. I suggest int as byte is not all that commonly used in cases where you're not handling binary data.
  • It took me several minutes to figure out what public enum Poss { sb, bb, btn, mid} was. Don't use (non-standard) abbreviations as type definitions. public enum Position { SmallBlind, BigBlind, DealerButton, None} would be much clearer. Some remarks:
    • Position has one S, so Poss is not an intuitive name at all.
    • Poss Pos You're using two different abbreviations for the same word. I suggest you avoid doing so. Be consistent.
    • I'm still not sure what mid is referring to. I assume this refers to "none of the other options".
  • I would completely remove the Position enum. Based on the dealer button alone, you can calculate who has the big and small blind. Using this enum just means that you have to manage all of these values and make sure you don't make mistakes. But if you calculate it, you don't run into the risk of forgetting to change one of the blinds.
    • The assignment of the dealer button should happen at the PokerTable level, not the PokerPlayer level. Imagine if a single player can play on two tables at the same time. Just because he has the dealer button on one table does not mean he had the dealer button on the other table.
    • A similar argument can be made about the chips. A player with 1000 chips in hand can choose to only place 250 chips on a particular poker table. However, I'm going to assume that this is simply out of scope for your current application. Over all, I do think that it'd be a good exercise for you to learn how to separate data that belongs to a player and data that belongs to a table.
  • Count most commonly refers to the amount of items in a list. ChipCount suggests that you have a list of Chip class objects. I would rename this to either Chips or ChipsAmount.
  • You can chain constructors, which would simplify your constructor code a bit.

I made a new chapter regarding Poss and BettingOrder because the changes there are too big to summarize here. The only thing that may warrant an example are the chained constructors:

    public PokerPlayer (int id)
    {
        ID = id;
    }
    public PokerPlayer(int id, string name) : this(id)
    {
        Name = name;
    }
    public PokerPlayer(int id, string name, int chipCount) : this(id, name)
    {
        ChipCount = chipCount;
    }

Furthermore, you seem to be creating constructors for every permutation of given parameters. In such a case, it's better to simply remove the constructors altogether, rely on a single parameterless constructor (when you can set some default values if you want to), and rely on (public) property initialization:

var player1 = new PokerPlayer() { Name = "Inky" };
var player2 = new PokerPlayer() { Name = "Pinky", ID = 5 };
var player3 = new PokerPlayer() { Name = "Blinky", ChipCount = 500 };
var player4 = new PokerPlayer() { Name = "Clyde", ID = 5, ChipCount = 1500 };

PlayerPosition, BettingOrder, and keeping the data easy to maintain

I made this a chapter in and of itself because it needs a major rework.

The initial cause of the issue was small. You chose to manually assign both the buttons (dealer, big blind, small blind) and the betting order to the players between every hand.
When you start the project from scratch, that seems like a simple first approach. However, you've stuck to that approach and have been writing a lot of code to implement this feature. The issue has grown to much larger proportions because it keeps being used everywhere.

You've skipped over a fairly important rule of efficient data management:

Calculated values should be calculated on the fly, not manually assigned and changed when needed.

The thing is, you've sort of following this rule when you calculated a card's suit and value based on its ID. Instead of setting the ID, suit and (card)value values; you set one of them and let the others be calculated from there.

You need to do the same here. Keep the manually mantained values to a minimum, and maximize the automatically calculated value.

Assuming you have an ordered list of players, and have a assigned a single player as the dealer, you can calculate everything else:

//Manually set
List<PokerPlayer> Players { get; set; }

int CurrentDealerIndex { get; set; }

//Calculated - but can be used to set the index by passing a PokerPlayer
PokerPlayer CurrentDealer 
{ 
    get { return Players[CurrentDealerIndex]; }
    set { CurrentDealerIndex = Players.IndexOf(value); }
}

Everything else can be calculated from this data:

  • Small blind player index = CurrentDealerIndex + 1;
  • Big blind player index = CurrentDealerIndex + 2;
  • Betting order can be found by starting with the small blind player, and moving through the player list (in order, cyclically) until you've retrieved every player once.

Note: You're going to have to use the % (modulo) operator when calculating with indexes in the list. I've omitted modulo calculation for the explanation, to keep the intention easy to understand.

There are many way to achieve this. Personally, I'd implement a cyclical list class, which removes the need for manually handling modulo operations. Here is one example implementation of such a list. You can find others on Google, but it'd also be a good exercise to try and create your own implementation.


PokerTable

  • Keep your properties in the same place. It took me a while to find List<PokerPlayer> PokerPlayers because you put in inbetween some methods.
  • I don't quite agree with the Streets enum. It's not necessary.
  • I do agree with the different methods, although I'd slightly rename them: DealHoleCards(), DealFlop(), DealTurn(), DealRiver().
  • throw new ArgumentOutOfRangeException("wrong street"); is ugly. This factors into the (in my opinion unnecessary) street enum. When you call the Flop() method, you should already know that it's time to play the flop! This exception would only be thrown if you were playing a flop when you shouldn't be, which is something that shouldn't ever occur in the first place. Your game logic is supposed to know which street it needs to play, not just make a guess.
    • I'm fairly certain that you added these exceptions specifically for debugging purposes when you were developing the logic. This can become a discussion topic, which I'd rather avoid. Using exceptions for debugging purposes is possible, but not every developer agrees that it is advisable.
  • A method that deals cards shoudn't also handle the betting round.
    • You may have missed this because you don't actually have betting rounds yet. You're simply adding an arbitrary amount to the pot.
  • Shuffling the deck doesn't quite belong to the table. I suggest making an intermediary Deck class which holds all the cards. This way, you encapsulate the card drawing/shuffling code into its own class, therefore allowing the PokerTable class to focus purely on table-related logic.
  • If you create a DoBettingRound() method, then you can define a single game as a sequence of the mentioned methods. This makes the code easy to read.

As a simple example:

public void PlayGame()
{
    this.Deck = new Deck(); //shuffled during initialization

    DealHoleCards();
    DoBettingRound();

    DealFlop();
    DoBettingRound();

    DealTurn();
    DoBettingRound();

    DealRiver();
    DoBettingRound();

    Showdown();
}

Note that this is an oversimplified example. You'll want to actually use method parameters here, as the first betting round (pre-flop) starts with a different player (big blind + 1) compared to the other betting rounds (which starts with the small blind player).
I suggest making the method public void PlayGame(PokerPlayer currentDealer) and then calculating the other players based on the current dealer.

\$\endgroup\$
  • \$\begingroup\$ Agree, thanks. Right now I have this mixed with a poker simulator so messy names. I will put it out into a its own project. I get the consensus is need Seats. Still going to give players directly at the table a try. The byte is lower case id. It is left over from a simulation where I run millions of boards and size matters. Agree on Chips and longer enum. Ctors are already cleaned up. But I think I do need to manually move the button and blinds every hand. A player could bust out so they will jump. I will look at cyclic. Right now it just deals the streets with fake bets. \$\endgroup\$ – paparazzo Mar 19 '18 at 14:08
  • \$\begingroup\$ @paparazzo: But I think I do need to manually move the button and blinds every hand. A player could bust out so they will jump. My suggestion is less different than you imply. You're still manually deciding to "pass" the dealer button, but you only pass the dealer button (based on the new game's player list - which may have changed between games) and leave the other assignments to be calculated on the fly based on that (changed) dealer assignment. Same functionality, less manual data management. \$\endgroup\$ – Flater Mar 19 '18 at 14:13
  • \$\begingroup\$ I need to add full betting at each street. I will break out Betting - working on that now. I don't want to create a deck each hand. Get better rand by shuffling the old and more efficient. But I get you point. Thanks again. \$\endgroup\$ – paparazzo Mar 19 '18 at 14:20
  • \$\begingroup\$ I come from running 20 million simulations. Not that pressed for time. I like the better random. I already broke out the shuffle. \$\endgroup\$ – paparazzo Mar 19 '18 at 14:24
  • \$\begingroup\$ If you use exceptions to catch stupid logic errors, consider assertions instead. They more explicitly say "you screwed up" (as opposed to "something broke"), and they're omitted in release mode (by which point the stupid errors are fixed anyway). \$\endgroup\$ – cHao Mar 19 '18 at 17:16
4
\$\begingroup\$

I've written a full version of this in C# to run multi-table tournaments, adjusting players and tables as players drop out etc. (I did it to test some strategies for different blind/time structures).

As you progress with this you will find there will be a lot of re-writing as you need to handle a multitude of different scenarios.

So to help with that I would suggest the following as a start:

  • Create classes for everything you can and use them as individual objects in the real world scenarios, only performing their own functionality (SOC).
  • Create a Card class that will hold the value, suit, and description of a single Card (11, "h", "Jh").
  • Create a Deck class that will hold the Cards (you may have a variant later that has different Cards).
  • Generate all 52 cards and put them in a Deck. (Multiple Tables will require multiple Decks).
  • Create a Shuffle routine on the Deck. Call it at the start of each hand.
  • Create a Dealer class that will hold the Deck and deal cards (at the least).
  • Create a Player class that has a Name and CurrentChips
  • Create a Pot class - there will be multiple pots so you need an array of them to track which Player is entitled to which Pot. Add to the Pot, take from the Pot as required.
  • Create a Seat, that will hold a Player, Position (SB, BB, BTN), current bet, and Chips.
  • Create a Table - that has an array of Seats (2 - 10), a Deck, Board Cards, a Pot array, a BTN, SB and BB position (rotating around the Seats), current blinds, current bet/raise, next Seat to act etc.
  • Add a Player to a Seat. Assume all of the Player's chips have been put on the Table (if a cash game then take chips from the Player and add them to their Seat).
  • Deal hole cards to the Seats.
  • Move the Blinds around the Table's Seats and get the bets from whatever Seats are in the Blinds.
  • When there is a bet, put the bet in the Pot, and remove that amount from the Seat.
  • When a hand is won by a Seat, add the applicable Pots' chips to the Seat's chips.
  • When a Player is moved from one Table to another, take the chips from the Seat, add them back to the Player's chips, remove the Player from the Seat. Now that the Seat is vacant, another player can just pop right in and drop their chips into the Seat and continue on. (The Dealer or Table don't care who's at the table, just the chips from a Seat).

Without classes for everything you can think of, there will be a mess of logic trying to keep track of what is happening on a Table. This way, you can keep the logic confined to each class, where they don't care what else is happening, they just do their own bit - and simply.

I hope this helps, and good luck with the project.

Edit

I think it has most of that stuff.

It may do, but it is all in a big lump of code. You need to break it down to the component parts.

Other answers have given suggestions about particular code issues and principals, so I'll stick to how you should be thinking about the whole poker scenario to break everything down.

You currently only have two classes to store all that is happening within a poker hand. And there is an awful lot more to go; comparing hands for winners, multi-way pots, bb/no sb hands, busting out, blind structure changing, tournament status, etc.

In PokerPlayer you have:

  • Position (Poss) (should be on the Table's Seat object)
  • ChipCount (should be the Player's chips in their pocket, but not the chips they have on the Table for the Hand. You can't take chips out of your pocket for a bet...)
  • Cards (should be in a Hand on the Seat. This would comprise the Player's hole cards added to the board cards)
  • BettingOrder (should be tracked on the Table by Seat)
  • An enum (Position) in every Player (should be global). There can be up to 10 positions, so selecting from four will limit your handling of the betting. Then use Positions on the Seats, not Players
  • A Player is part of a Table, they sit at a Seat, and the Seat is part of a Table

In PokerTable you have:

  • An enum (Streets) in every Table (should be global)

  • DealerName (Players aren't dealers, a dealer joins a Table to deal, and a dealer button moves around the table to indicate which Seat is last to act)

  • Shuffle (shuffling is a function of the Deck, should be on the Deck object, or better still a separate function that takes a Deck and returns it shuffled)

  • Flop, Turn, River (when dealing a Card, 'remove' it from the Deck. Don't just increment a counter; if you miss an increment, or increment too many times, you will have unexpected results).

You need to think of all of these things as real world objects to break them down into their component parts and functionality (SOC - Separation of Concerns, SRP - Single Responsibility Principal).

Thinking like this:

  • A player can move from table to table

  • A deck of cards can be thrown away and another one used

  • A deck is shuffled, nothing else shuffles the deck

  • A players does not move position around a table

  • A dealer button moves around the seats on a table

Will lead you to creating a class for each 'thing' you need to work with.

Example:

Create a Table that has:

  • A collection of Seats
  • A collection of Pots
  • A Deck
  • A Board
  • Tracking for current bet/raise
  • Tracking for current Seat to bet
  • Tracking for current Seats still in the hand

And break these down further:

  • A Seat has status; occupied, out of hand, chips remaining, current bet, etc.
  • A Deck has Cards that can be shuffled, and only has the Cards remaining that haven't been dealt yet
  • A Shuffle routine that accepts a Deck and returns it shuffled
\$\endgroup\$
  • 1
    \$\begingroup\$ I think it has most of that stuff. Do you have any comments on the existing code. \$\endgroup\$ – paparazzo Mar 19 '18 at 4:55
  • \$\begingroup\$ Thanks for the excellent feedback. For betting the rules are the same for all mid positions. Current Dealer is not a player. I don't plan to mess up cardNum. Reshuffle an existing shuffled deck is more efficient and would make hacking the rand more difficult (if not near impossible). This is just a start - I know there is more to the game. I know you feel strongly about Seat. I considered it and for now just going with an an ordered List of Players. Say I want to host heads up games - why reserve 10 seats. It is less bandwidth. I do not plan to do multi table tournaments. \$\endgroup\$ – paparazzo Mar 19 '18 at 13:50
  • \$\begingroup\$ OK, if you've got a target for functionality now, that's fair. But in the future when you need to expand it, it will be harder if it's not separated out at the start. I've not seen an app yet that didn't grow after it accomplished it's first goal... \$\endgroup\$ – Steve Padmore Mar 19 '18 at 14:25
  • \$\begingroup\$ I am with you and a little torn. If I want to do multi-table tournaments in the future there is some stuff I would do different but I don't plan on doing multi-table tournaments. \$\endgroup\$ – paparazzo Mar 19 '18 at 14:34
  • 1
    \$\begingroup\$ I too, have written several Poker-Playing programs and ultimately my object models ended up looking much more like this than that in the question. in particular, I always ended up with a Deck class that was separate from the Table and a Hand class that was separate from the Player class. IMHO, it supports better SRP and ultimately makes the code simpler and easier to both understand and edit. \$\endgroup\$ – RBarryYoung Mar 19 '18 at 19:06
0
\$\begingroup\$

The code does not test for number of players. Need to have at least 2 players. A standard table is limited to 10 players. A deck would support 23 players.

The fixed blinds of 1 and 2 would make it difficult to upgrade to a tournament where the blinds increase. No seat could also make moving tables and joining tables more difficult. For now a multi table tournament is out of scope.

There is no test that player names are unique.

\$\endgroup\$
  • \$\begingroup\$ As far as uniqueness of names...There's also nothing in poker saying 9 guys all named John Doe couldn't play at the same table. If you depend on names for anything but display purposes, you're doing something wrong. \$\endgroup\$ – cHao Mar 19 '18 at 17:29
  • \$\begingroup\$ @cHao I don't have the DB yet to supply a unique ID so for now I am using Name. \$\endgroup\$ – paparazzo Mar 19 '18 at 17:35
  • \$\begingroup\$ You'd be surprised how often "for now" turns into "for so long that it's now baked in and the code now depends on it". Don't implement assumptions you know aren't valid. You don't need a DB for unique IDs -- Guids exist. \$\endgroup\$ – cHao Mar 19 '18 at 17:41
  • \$\begingroup\$ @cHao I will have proper int ID shortly. A db is not optional. \$\endgroup\$ – paparazzo Mar 19 '18 at 17:52
  • \$\begingroup\$ Then there's no reason to add a test for unique names. \$\endgroup\$ – cHao Mar 19 '18 at 18:03
-2
\$\begingroup\$

You don't really need a shuffle method. You can simply have a deck object with a draw method that randomly picks a card from the deck. Whenever you need a card, draw a card from the deck, and part of the draw method is removing the appropriate card from the deck. If you have eight players, you'll need to draw 21 cards. I suppose conceptually it's nice to "shuffle" the 21 cards you need before dealing any out, and to include the other 31 cards in the "shuffle", but I personally don't see much value in doing all that work just to imitate more closely how "real world" poker is implemented.

You seem to be using bb for both the position and the value. You should distinguish between the two, e.g. bb_player versus bb_size. The line Pot = sb + bb is really weird. Apparently you're initializing the pot with the blinds, but are you removing the blinds from anyone's chip stack? You should have a bet method, and "force" the two blind players to call that method. Also, having a method named "Shuffle" that does a bunch of stuff other than shuffling, such as initializing the pot, is bad naming.

I think that betting order should be an attribute of the table rather than the players. There's no need for a player object to know what its betting order is; if a player wants that sort of information, you can just pass them the entire table configuration (that is, I can see a player asking "What's the betting order", but I can't see a player asking just "What's my betting order?"). The table should have a blind method that adjusts chip totals for bb and sb, and it should have a method that asks the current player to act. Both of those methods should know what the betting order is, and be telling players when it's their turn to act. If the player objects know who is sitting to their left and right, then you can just have the table ask for a player to act, and then ask for the next player to act, etc. Instead of doing btn++, you can do button_player = button_player.left, and then you don't have to do if(btn == PokerPlayers.Count){btn = 0;}. Under the hood, this is more complicated, but conceptually it's simpler; the button just moves to the left. Other things such as the betting action can move similarly.

Adding 2*cnt to a number and then taking the result mod cnt is a bit odd; (x+k*y) % y should give x for every k.

\$\endgroup\$
  • 2
    \$\begingroup\$ There are very very good reason to shuffle the whole deck. This is how commercial sites do it. Rand at a time and remove is less efficient and less random. I am a mathematician. C# is case sensitive. I said fake bet and bets are not yet implemented! It is very important for a player to know where they are in the order. I will guess you don't play poker. Please test without 2*cnt. Why should a server notify a player to act. Have a queue to accept action so they can do stuff like fold in advance and speed up the game. That is how poker sites work. \$\endgroup\$ – paparazzo Mar 19 '18 at 20:26
  • \$\begingroup\$ 1. Picking a card at a time isn't less random, and it looked like that's how you were doing it, anyway. 2. A blind is a bet. 3. As I said, a player would want to see the table, I don't see why they would want to see just their position. 4. Can you give an example when 2*cnt gives a different answer? 5. The server should notify the player object. \$\endgroup\$ – Acccumulation Mar 19 '18 at 20:54
  • \$\begingroup\$ The suggestion not to shuffle is a horrible idea. To remove cards from a deck implies there will be 2 lists: the original and the subset with cards removed. \$\endgroup\$ – Rick Davin Mar 20 '18 at 14:56
  • \$\begingroup\$ @RickDavin There aren't necessarily two lists, and if there are, so what? The set of all cards is constant. And you'll need to keep track of the whole deck to shuffle anyway. \$\endgroup\$ – Acccumulation Mar 20 '18 at 15:01
  • \$\begingroup\$ @paparazzo: It's how poker clients work. But it's likely not how poker sites work, and it's not how poker itself works -- players can pre-decide, but don't act (and etiquette demands they not even state their next act) before their turn. The client should remember the next move and supply it on the player's turn. The reason for notification is to separate concerns. It gets the server out of the action-buffering business, and clients no longer care about betting order, just seat #. Notifications also provide clients a way to relay game state more reliably. \$\endgroup\$ – cHao Mar 20 '18 at 16:04

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.