1
\$\begingroup\$

I want to make a simple poker game.

What I pay particular attention to is design:

I look for suggestions to improve the quality of the design and also suggestions on possible improvements in style and logic.

Now the code that working good :

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Poker_Calc_EnglishVersion
{
  public enum Suit
  {
    Hearts = 0,
    Diamonds,
    Clubs,
    Spades
  };

  public enum Value
  {
    A = 1,
    Two,
    Three,
    Four,
    Five,
    Six,
    Seven,
    Eight,
    Nine,
    T,
    J,
    Q,
    K
  };

  public class Card
  {
    public Suit Suid { get; }
    public Value Value { get; }



    public Card(Value value, Suit suit)
    {
        Value = value;
        Suid = suit;
    }



    public override string ToString()
    {
        //return base.ToString()+ " Value: " + Value + " Suit: " + Suid;

        return " Value: " + Value + " Suit: " + Suid;
    }
 }

Deck :

public class Deck
{

    private List<Card> _deck;
    public int _deckDimension { get; }

    public Deck()
    {
        this._deckDimension = 52;
        this._deck = new List<Card>(_deckDimension);

        foreach (Suit s in Enum.GetValues(typeof(Suit)))
        {
            foreach (Value v in Enum.GetValues(typeof(Value)))
            {
                Card c = new Card(v, s);
                _deck.Add(c);
            }
        }
    }


    public Card GetCard(int i)
    {
        if (!(i >= 0 && i < _deckDimension))
        {
            return null;
        }
        return _deck[i];
    }

    public Card RemoveCard(int pos)
    {
        if (!(pos >= 0 && pos < _deckDimension))
        {
            return null;
        }

        Card cd_return = GetCard(pos);
        _deck.RemoveAt(pos);

        return cd_return;
    }


    public void SwapCard(int a, int b)
    {
        Card cd = GetCard(a);
        _deck[a] = GetCard(b);
        _deck[b] = cd;

    }

Dealer :

public class Dealer
{
    private Deck _deck;

    public Dealer(Deck deck)
    {
        this._deck = deck;
    }


    public bool Shuffle()
    {
        var rnd = new Random();
        int i = 0;

        int first;
        int second;

        while (i < 10000)
        {
            first = rnd.Next(0, _deck._deckDimension - 1);
            do
            {
                second = rnd.Next(0, _deck._deckDimension - 1);
            }
            while (first == second);

            _deck.SwapCard(first, second);
            i++;
        }

        return true;
    }


    public Card DealCard()
    {
        Random rnd = new Random();
        int first;
        first = rnd.Next(0, _deck._deckDimension - 1);

        return _deck.RemoveCard(first);

    }

Player

public class Player
{
    private float _stack { get; }
    private List<Card> _hand { get; }

    public Player()
    {
        _hand = new List<Card>(2);
    }


    public void ReceiveCard(Card c)
    {
        _hand.Add(c);
    }


    public List<Card> GetHand()
    {
        return _hand;
    }

Button1 and some listbox for testing and see result

    private void Button1_Click(object sender, EventArgs e)
    {
        Deck deck = new Deck();

        deck.PrintDeck(listBox1);

        Dealer dl = new Dealer(deck);

        dl.Shuffle();

        deck.PrintDeck(listBox2);

        label1.Text = deck._deckDimension.ToString();

        dl.OrderDeck();

        deck.PrintDeck(listBox3);

        Player p1 = new Player();

        p1.ReceiveCard(dl.DealCard());
        //thread.sleep use for switch context and random generate not a two consecutive card
        System.Threading.Thread.Sleep(50);
        p1.ReceiveCard(dl.DealCard());

        p1.PrintHand(listBox4);


    }

There are some methods like PrintDeck() and Order() that I have created only for testing..

I know there is a "Fisher Yates" algorithm for Shuffle() I will implement it later.

How would you improve this code? How would you change the class design??

I have repeatedly asked myself if it makes sense to create a Hand class with two fields Card.

It makes sense because it is part of the context we are "modeling" and could contain the logic for the comparison (Hand valuation)

I await advice and opinions about it and for the general improvement of the code.

The next step is

  • Create a functionality for distribute a Flop, Turn and River
  • Create a functionality for Hand Valuation
  • Create a functionality for Calculate odds when player goes all in before river coming
  • And so on ..
\$\endgroup\$
2
\$\begingroup\$

Deck

In both methods, GetCard() and RemoveCard(), the method-argument-validation is hard to read at first glance. You should change the condition and maybe extract the validation into its own method.

Having the condition like this

public Card GetCard(int i)
{
    if (i < 0 || i >= _deckDimension)
    {
        return null;
    }
    return _deck[i];
}  

is much easier to read. But wait, _deckDimension isn't a field, its a property and therfor it should not be named using underscore-prefix. Based on the .NET Naming Guidelines methods should be named using PascalCase casing. _deckDimension -> DeckDimension.

But still, this doesn't look right. If the validation fails the methods shouldn't return null but throwing an ArgumentOutOfRangeException.

Looking at RemoveCard() I think you really should read the .NET Naming Guidelines. In .NET one shouldn't use snake_case casing for naming things and shouldn't use abbreviations like e.g cd_return. Why don't you name it card?

Dealer

You are passing the Deck into the constructor, which is a good thing, but, because you don't change the _deck you should make it readonly.

The Random class in .NET Framework isn't that random. If you call e.g the Shuffle() method very often and fast, it is possible to get the same random values over and over again. This is because Random when instantiated will use the system clock to provide a seed value. You should have one Random object as a class-level field which should be instantiated in the constructor or directly.

The second argument of Random.Next(int, int) is the exclusive upper bound of the random number to be returned. By calling .Next(0, _deck._deckDimension - 1 you get 0 <= number < 51 but you want the card with number 51 as well, don't you? Thats true for DealCard() as well.

Making Shuffle() void would be good as well. I can't think of a way that shuffeling could fail, at least for a computer-dealer. A human could throw the cards on the ground, a computer wouldn't be so clumsy.

Its possible that DealCard() throws an ArgumentOutOfRangeException in its current implementation. Let us assume we call DealCard() the first time and the call to Randum.Next() returns 1 you return the second Card in _deck. Calling DealCard() again, the call to Randum.Next() returns 1 again, no problem you return the second Card in _deck again. The third call to DealCard results in Random.Next() returning 51 but you only have 50 cards left in the deck, resulting in the said exception in Deck.GetCard().

To prevent this to happen we need to adjust the validation inside GetCard() and RemoveCard(). Instead of checking _deckDimension we should check against _deck.Count like so

public Card GetCard(int i)
{
    if (i < 0 || i >= _deck.Count || _deck.Count == 0)
    {
        throw new ArgumentOutOfRangeException(nameof(i));
    }
    return _deck[i];
}  

and change the call to Random.Next() in DealCard like so

first = rnd.Next(0, _deck.Count);

You can declare and initialize a variable on the same line. E.g in DealCard() you have

int first;
first = rnd.Next(0, _deck._deckDimension - 1);  

which should be changed to

int first = rnd.Next(0, _deck._deckDimension - 1);   

I just wanted to write that you should use var instead of the concrete type in DealCard() but had a quick glance at Shuffle() again where you used var. The rule of thumb for everything is be consistent. Choose one style and stick to it.

Use either var rnd = new Random(); or Random rnd = new Random(); but don't use both.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ty @Heslacher :) Do you have a suggestions about the design ? Do you have a tip for me for improve a check on Swap() ,RemoveCard() and Shuffle()? Shuffle return always true without check, isn't a good. For example , make sense to control the number of card in _RemoveCard() before method return ? how to check that the card was actually removed? How would you improve these parts? \$\endgroup\$ – rul3z Dec 19 '19 at 5:12
  • 1
    \$\begingroup\$ because in the _deck there is 52 Card. From 0 to 51... \$\endgroup\$ – rul3z Dec 22 '19 at 13:44
  • \$\begingroup\$ I have edit Deck class. Now there is only 'List<Card> _deck' such as field. Not others. I have implements a property without a field ' public int Dimension { get { return _deck.Count; } } ' . Is corret to have a one property without a field ?? I don't know but is a obtain value by '_deck.Count'. Now I continue to use in Dealer class '_deck.Dimension -1 ' for check in 'Shuffle() ' and ' DealCard() ' methods but now shouldn't be problems with an 'OutOfRange' ... What do you think about this solution ?? ty! @Heslacher \$\endgroup\$ – rul3z Dec 22 '19 at 22:31
  • 1
    \$\begingroup\$ Please work a little bit longer on your application meaning make it more finished and then come here again and ask a new question. \$\endgroup\$ – Heslacher Dec 23 '19 at 5:42

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.