2
\$\begingroup\$

I want to clean this code to be more production ready. What can be improved, added or removed?

 enum Suits {Spades, Hearts, Diamonds, Clubs}
 class Card {

    public string Value { set; get;}
    public Suits Suit {get; set;}
 }


 class Player
 {
     public string Name { get; set; }
     public List<Card> Cards { get; set; }


     public int CalculatePoints()
     {
         int points = 0;
         int aces = 0;

         foreach (Card c in Cards)
         {
             if (c.Value == "A")
             {
                 aces++;
                 // points++;
             }
             else if (c.Value == "J" || c.Value == "Q" || c.Value == "K") points += 10;
             else points += int.Parse(c.Value);
         }

         if ((points - 21) <= 10)
         {
             for (int j = 0; j < aces; j++)
             {
                 if (j == 0) points += 11; // Adding at least one 11
                 else points += 1;  // the rest can only be 1s
             }
         }
         else
         {
             points += aces; //if we do not have a space for an 11 then lets just add 1s
         }

         return points;
     }
 }

 class Program
 {
     static void Main(string[] args)
     {
         Player Dealer = new Player
         {
             Name = "Dealer",
             Cards = new List<Card> {
                new Card{Value= "J", Suit = Suits.Spades},
                new Card{Value= "9", Suit = Suits.Hearts}
             }
         };

         Player Billy = new Player
         {
             Name = "Billy",
             Cards = new List<Card>{
                new Card{Value="2", Suit = Suits.Spades},
                new Card{Value="2", Suit = Suits.Diamonds},
                new Card{Value="2", Suit = Suits.Hearts},
                new Card{Value="4", Suit = Suits.Diamonds},
                new Card{Value="5", Suit = Suits.Clubs}
            }
         };

         Player Lemmy = new Player
         {
             Name = "Lemmy",
             Cards = new List<Card>{
                new Card{Value="A", Suit = Suits.Spades},
                new Card{Value="7", Suit = Suits.Hearts},
                new Card{Value="A", Suit = Suits.Diamonds}
            }
         };

         Player Andrew = new Player
         {
             Name = "Andrew",
             Cards = new List<Card>{
                new Card{Value="K", Suit = Suits.Diamonds},
                new Card{Value="4", Suit = Suits.Spades},
                new Card{Value="4", Suit = Suits.Clubs}
            }
         };

         Player Carla = new Player
         {
             Name = "Carla",
             Cards = new List<Card>{
                new Card{Value="Q", Suit = Suits.Clubs},
                new Card{Value="6", Suit = Suits.Spades},
                new Card{Value="9", Suit = Suits.Diamonds}
            }
         };

         Console.WriteLine("Dealer's Points = {0}", Dealer.CalculatePoints());
         Console.WriteLine("==========================");
         //Output the winner
         Console.WriteLine(String.Format("Billy's Points   = {0}   |  {1}", Billy.CalculatePoints(), getWinner(Dealer, Billy)));
         Console.WriteLine(String.Format("Lemmy's Points   = {0}   |  {1}", Lemmy.CalculatePoints(), getWinner(Dealer, Lemmy)));
         Console.WriteLine(String.Format("Andrew's Points  = {0}   |  {1}", Andrew.CalculatePoints(), getWinner(Dealer, Andrew)));
         Console.WriteLine(String.Format("Carla's Points   = {0}   |  {1}", Carla.CalculatePoints(), getWinner(Dealer, Carla)));
         Console.ReadLine();

     }

     //Calculate the winner
     private static string getWinner(Player dealer, Player player)
     {
         if (player.CalculatePoints() > 21)
         {
             return "Looses";
         }
         else if (dealer.CalculatePoints() > 21)
         {
             return "Wins";
         }
         else if (player.Cards.Count == 5)
         {
             return "Wins : Has 5 cards";
         }
         return dealer.CalculatePoints() > player.CalculatePoints() ? "Looses" : "Wins";
     }
 }
\$\endgroup\$
  • \$\begingroup\$ Thanks,Can anyone assist me? \$\endgroup\$ – user4261104 Oct 29 '18 at 11:58
2
\$\begingroup\$

I'm not familiar with the rules of Black Jack so I won't comment the calculations.

You should have an enum for faces as you have for suits:

enum Suits { Spades, Hearts, Diamonds, Clubs }
enum Faces { Two = 2, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace }

class Card
{

  public Faces Face { set; get; }
  public Suits Suit { get; set; }
}

It is easier to read and is less error prone.


The Player could have a Points property that could be recalculated by CalculatePoints() when necessary to avoid the overhead when recalculating it every time it's needed but not necessarily changed:

class Player
{
  public string Name { get; set; }
  public List<Card> Cards { get; set; }

  public int Points { get; private set; }

  public void CalculatePoints()
  {
    int points = 0;
    int aces = 0;

    foreach (Card c in Cards)
    {
      if (c.Face == Faces.Ace)
      {
        aces++;
        // points++;
      }
      else if (c.Face == Faces.Jack || c.Face == Faces.Queen || c.Face == Faces.King) points += 10;
      else points += (int)c.Face;
    }

    if ((points - 21) <= 10)
    {
      for (int j = 0; j < aces; j++)
      {
        if (j == 0) points += 11; // Adding at least one 11
        else points += 1;  // the rest can only be 1s
      }
    }
    else
    {
      points += aces; //if we do not have a space for an 11 then lets just add 1s
    }

    Points = points;
  }
}

... but that may be a bad design to have the client to recalculate the Points when needed. Instead you can hide the list of Cards so the Player can control when the list of cards changes and recalculate the Points accordingly:

class Player : IEnumerable<Card>
{
  private List<Card> m_cards = new List<Card>();

  public string Name { get; set; }
  public int Points { get; private set; }
  public int CountOfCards => m_cards.Count;

  private void AddOneCard(Card card)
  {
    if (!m_cards.Contains(card))
    {
      m_cards.Add(card);
    }
  }

  public void AddCard(Card card)
  {
    AddOneCard(card);
    CalculatePoints();
  }

  public Player AddRange(params Card[] cards)
  {
    foreach (Card card in cards)
    {
      AddOneCard(card);
    }

    CalculatePoints();

    return this;
  }

  public bool RemoveCard(Card card)
  {
    bool result = m_cards.Remove(card);
    CalculatePoints();
    return result;
  }

  private void CalculatePoints()
  {
    int points = 0;
    int aces = 0;

    foreach (Card c in m_cards)
    {
      if (c.Face == Faces.Ace)
      {
        aces++;
        // points++;
      }
      else if (c.Face == Faces.Jack || c.Face == Faces.Queen || c.Face == Faces.King) points += 10;
      else points += (int)c.Face;
    }

    if ((points - 21) <= 10)
    {
      for (int j = 0; j < aces; j++)
      {
        if (j == 0) points += 11; // Adding at least one 11
        else points += 1;  // the rest can only be 1s
      }
    }
    else
    {
      points += aces; //if we do not have a space for an 11 then lets just add 1s
    }

    Points = points;
  }

  public IEnumerator<Card> GetEnumerator()
  {
    return m_cards.GetEnumerator();
  }

  IEnumerator IEnumerable.GetEnumerator()
  {
    return GetEnumerator();
  }
}

In this way the Player controls it hand of cards and the surroundings can view, but not modify it without the Players knowledge.

A Player can then be created like this:

  Player dealer = new Player { Name = "Dealer" }.AddRange(
    new Card { Face = Faces.Jack, Suit = Suits.Spades },
    new Card { Face = Faces.Nine, Suit = Suits.Hearts });

getWinner()

should be named in PascalCase:

private static string GetWinner(Player dealer, Player player) {}

It's OK to have the dealer as a seperate variable, but you can simplify the code, if you create an array or list for the players:

  List<Player> players = new List<Player>
  {
    new Player
    {
      Name = "Billy",
      Cards = new List<Card>
      {
        new Card{Face= Faces.Two, Suit = Suits.Spades},
        new Card{Face= Faces.Two, Suit = Suits.Diamonds},
        new Card{Face= Faces.Two, Suit = Suits.Hearts},
        new Card{Face= Faces.Four, Suit = Suits.Diamonds},
        new Card{Face= Faces.Five, Suit = Suits.Clubs}
      }
    },
    new Player
    {
      Name = "Lemmy",
      Cards = new List<Card>
      {
        new Card{Face= Faces.Ace, Suit = Suits.Spades},
        new Card{Face= Faces.Seven, Suit = Suits.Hearts},
        new Card{Face= Faces.Ace, Suit = Suits.Diamonds}
      }
    },
    new Player
    {
      Name = "Andrew",
      Cards = new List<Card>
      {
        new Card{Face= Faces.King, Suit = Suits.Diamonds},
        new Card{Face= Faces.Four, Suit = Suits.Spades},
        new Card{Face= Faces.Four, Suit = Suits.Clubs}
      }
    },
    new Player
    {
      Name = "Carla",
      Cards = new List<Card>
      {
        new Card{Face= Faces.Queen, Suit = Suits.Clubs},
        new Card{Face= Faces.Six, Suit = Suits.Spades},
        new Card{Face= Faces.Nine, Suit = Suits.Diamonds}
      }
    }
  };

You can then display the result like this:

  dealer.CalculatePoints();
  Console.WriteLine("Dealer's Points = {0}", dealer.Points);
  Console.WriteLine("==========================");

  foreach (Player player in players)
  {
    player.CalculatePoints();
    Console.WriteLine($"{$"{player.Name}'s", -10} Points   = {player.Points}   |  {GetWinner(dealer, player)}");
  }
  Console.ReadLine();

You should of course divide the main() method into meaningful methods like:

static void Main() 
{
  Player dealer = CreatePlayer("dealer");
  List<Player> players = CreatePlayers("Billy", "Lemmy", "Andrew", "Carla");
  ShowResult(dealer, players);
}

static Deck deck = new Deck();

static Player CreatePlayer(string name)
{
  // TODO: create a player and select cards from a `Deck` of cards randomly
}

static List<Player> CreatePlayers(params string[] names)
{
   return names.Select(name => CreatePlayer(name)).ToList();
}

class Deck
{
  Stack<Card> _cards = new Stack<Card>(); // Jokers???

  private void Initialize() {...}

  public void Shuffle() { // TODO: shuffle the card stack randomly }

  public Card Pop() { return _cards.Pop(); }

  // ... etc.
}
\$\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.