Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What do you think the weak points are and how can I improve? I thought I could really use a Hand class in the dealHand method.

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

namespace ConsoleBlackjack
{
    public enum Suit
    {
        Heart,
        Diamond,
        Spade,
        Club
    }

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

    public class Card
    {
        public Suit Suit { get; set; }
        public Face Face { get; set; }
        public int Value { get; set; }
    }

    public class Deck
    {
        private List<Card> cards;

        public Deck()
        {
            this.Initialize();
        }

        public void Initialize()
        {
            cards = new List<Card>();

            for (int i = 0; i < 4; i++)
            {
                for (int j = 0; j < 13; j++)
                {
                    cards.Add(new Card() { Suit = (Suit)i, Face = (Face)j });

                    if (j <= 8)
                        cards[cards.Count - 1].Value = j + 1;
                    else 
                        cards[cards.Count - 1].Value = 10;
                }
            }
        }

        public void Shuffle()
        {
            Random rng = new Random();
            int n = cards.Count;
            while (n > 1)
            {
                n--;
                int k = rng.Next(n + 1);
                Card card = cards[k];
                cards[k] = cards[n];
                cards[n] = card;
            }
        }

        public Card DrawACard()
        {
            if (cards.Count <= 0)
            {
                this.Initialize();
                this.Shuffle();
            }

            Card cardToReturn = cards[cards.Count - 1];
            cards.RemoveAt(cards.Count - 1);
            return cardToReturn;
        }

        public int GetAmountOfRemainingCrads()
        {
            return cards.Count;
        }

        public void PrintDeck()
        {
            int i = 1;
            foreach (Card card in cards)
            {
                Console.WriteLine("Card {0}: {1} of {2}. Value: {3}", i, card.Face, card.Suit, card.Value);
                i++;
            }
        }



    }

    class Program
    {
        static int chips;
        static Deck deck;
        static List<Card> userHand;
        static List<Card> dealerHand;

        static void Main(string[] args)
        {
            Console.Title = "♠♥♣♦ Blackjack Game by Niv Harel";
            Console.WriteLine("Welcome to blackjack 0.1a\n");

            chips = 100;
            deck = new Deck();
            deck.Shuffle();

            while (chips > 0)
            {
                DealHand();
                Console.WriteLine("\nPress any key for the next hand...\n");
                ConsoleKeyInfo userInput = Console.ReadKey(true);
            }

            Console.WriteLine("You Lost! see you next time...");
            Console.ReadLine();
        }

        static void DealHand()
        {
            if (deck.GetAmountOfRemainingCrads() < 20)
            {
                deck.Initialize();
                deck.Shuffle();
            }

            Console.WriteLine("Remaining Cards: {0}", deck.GetAmountOfRemainingCrads());
            Console.WriteLine("Current Chips: {0}", chips);
            Console.WriteLine("How much would you like to bet? (1 - {0})", chips);
            string input = Console.ReadLine().Trim().Replace(" ","");
            int betAmount;
            while (!Int32.TryParse(input, out betAmount) || betAmount < 1 || betAmount > chips )
            {
                Console.WriteLine("amount is illegal. How much would you like to bet? (1 - {0})", chips);
                input = Console.ReadLine().Trim().Replace(" ", "");
            }
            Console.WriteLine();

            userHand = new List<Card>();
            userHand.Add(deck.DrawACard());
            userHand.Add(deck.DrawACard());

            foreach (Card card in userHand)
            {
                if (card.Face == Face.Ace)
                {
                    card.Value += 10;
                    break;
                }
            }

            Console.WriteLine("[Player]");
            Console.WriteLine("Card 1: {0} of {1}", userHand[0].Face, userHand[0].Suit);
            Console.WriteLine("Card 2: {0} of {1}", userHand[1].Face, userHand[1].Suit);
            Console.WriteLine("Total: {0}\n", userHand[0].Value + userHand[1].Value);

            dealerHand = new List<Card>();
            dealerHand.Add(deck.DrawACard());
            dealerHand.Add(deck.DrawACard());

            foreach (Card card in dealerHand)
            {
                if (card.Face == Face.Ace)
                {
                    card.Value += 10;
                    break;
                }
            }

            Console.WriteLine("[Delaer]");
            Console.WriteLine("Card 1: {0} of {1}", dealerHand[0].Face, dealerHand[1].Suit);
            Console.WriteLine("Card 2: [Hole Card]");
            Console.WriteLine("Total: {0}\n", dealerHand[0].Value);

            bool insurance = false; ;

            if (dealerHand[0].Face == Face.Ace)
            {
                Console.WriteLine("Insurance? (y / n)");
                string userInput = Console.ReadLine();

                while (userInput != "y" && userInput != "n")
                {
                    Console.WriteLine("Could not understand. Insurance? (y / n)");
                    userInput = Console.ReadLine();
                }

                if (userInput == "y")
                {
                    insurance = true;
                    //chips -= betAmount / 2;
                    Console.WriteLine("\n[Insurance Accepted!]\n");
                }
                else
                {
                    insurance = false;
                    Console.WriteLine("\n[Insurance Rejected]\n");
                }
            }

            if (dealerHand[0].Face == Face.Ace || dealerHand[0].Value == 10)
            {
                Console.WriteLine("Delaer checks if he has blackjack...\n");
                Thread.Sleep(2000);
                if (dealerHand[0].Value + dealerHand[1].Value == 21)
                {
                    Console.WriteLine("[Delaer]");
                    Console.WriteLine("Card 1: {0} of {1}", dealerHand[0].Face, dealerHand[1].Suit);
                    Console.WriteLine("Card 2: {0} of {1}", dealerHand[1].Face, dealerHand[1].Suit);
                    Console.WriteLine("Total: {0}\n", dealerHand[0].Value + dealerHand[1].Value);

                    Thread.Sleep(2000);

                    int amountLost = 0;

                    if (userHand[0].Value + userHand[1].Value == 21 && insurance)
                    {
                        amountLost = betAmount / 2;
                        chips -= betAmount / 2;
                    }
                    else if (userHand[0].Value + userHand[1].Value != 21 && !insurance)
                    {
                        amountLost = betAmount + betAmount / 2;
                        chips -= betAmount + betAmount / 2;
                    }

                    Console.WriteLine("You lost {0} chips", amountLost);

                    Thread.Sleep(1000);

                    return;
                }
                else
                {
                    Console.WriteLine("Dealer does not have a blackjack, moving on...\n");
                }
            }

            if (userHand[0].Value + userHand[1].Value == 21)
            {
                Console.WriteLine("Blackjack, You Won! ({0} chips)\n", betAmount + betAmount / 2);
                chips += betAmount + betAmount / 2;
                return;
            }

            do
            {
                Console.WriteLine("Please choose a valid option: [(S)tand (H)it]");
                ConsoleKeyInfo userOption = Console.ReadKey(true);
                while (userOption.Key != ConsoleKey.H && userOption.Key != ConsoleKey.S)
                {
                    Console.WriteLine("illegal key. Please choose a valid option: [(S)tand (H)it]");
                    userOption = Console.ReadKey(true);
                }
                Console.WriteLine();

                switch (userOption.Key)
                {
                    case ConsoleKey.H:
                        userHand.Add(deck.DrawACard());
                        Console.WriteLine("Hitted {0} of {1}", userHand[userHand.Count - 1].Face, userHand[userHand.Count - 1].Suit);
                        int totalCardsValue = 0;
                        foreach (Card card in userHand)
                        {
                            totalCardsValue += card.Value;
                        }
                        Console.WriteLine("Total cards value now: {0}\n", totalCardsValue);
                        if (totalCardsValue > 21)
                        {
                            Console.Write("Busted!\n");
                            chips -= betAmount;
                            Thread.Sleep(2000);
                            return;
                        }
                        else if (totalCardsValue == 21)
                        {
                            Console.WriteLine("Good job! I assume you want to stand from now on...\n");
                            Thread.Sleep(2000);
                            continue;
                        }
                        else
                        {
                            continue;
                        }
                        break;

                    case ConsoleKey.S:

                        Console.WriteLine("[Delaer]");
                        Console.WriteLine("Card 1: {0} of {1}", dealerHand[0].Face, dealerHand[1].Suit);
                        Console.WriteLine("Card 2: {0} of {1}", dealerHand[1].Face, dealerHand[1].Suit);

                        int dealerCardsValue = 0;
                        foreach (Card card in dealerHand)
                        {
                            dealerCardsValue += card.Value;
                        }

                        while (dealerCardsValue < 17)
                        {
                            Thread.Sleep(2000);
                            dealerHand.Add(deck.DrawACard());
                            dealerCardsValue = 0;
                            foreach (Card card in dealerHand)
                            {
                                dealerCardsValue += card.Value;
                            }
                            Console.WriteLine("Card {0}: {1} of {2}", dealerHand.Count, dealerHand[dealerHand.Count - 1].Face, dealerHand[dealerHand.Count - 1].Suit);
                        }
                        dealerCardsValue = 0;
                        foreach (Card card in dealerHand)
                        {
                            dealerCardsValue += card.Value;
                        }
                        Console.WriteLine("Total: {0}\n", dealerCardsValue);

                        if (dealerCardsValue > 21)
                        {
                            Console.WriteLine("Dealer bust! You win! ({0} chips)", betAmount);
                            chips += betAmount;
                            return;
                        }
                        else
                        {
                            int playerCardValue = 0;
                            foreach(Card card in userHand)
                            {
                                playerCardValue += card.Value;
                            }

                            if (dealerCardsValue > playerCardValue)
                            {
                                Console.WriteLine("Dealer has {0} and player has {1}, dealer wins!", dealerCardsValue, playerCardValue);
                                chips -= betAmount;
                                return;
                            }
                            else
                            {
                                Console.WriteLine("Player has {0} and dealer has {1}, player wins!", playerCardValue, dealerCardsValue);
                                chips += betAmount;
                                return;
                            }
                        }
                        break;

                    default:
                        break;
                }

                Console.ReadLine();
            }
            while (true);
        }
    }
}
share|improve this question
5  
I don't know if I'll have time to come back (I hope I do), but I have to point out that you should not be able to set the value or face of a card after it's created. Also, any real game of blackjack is played with at least 3 full 52 card decks. –  RubberDuck Aug 17 at 20:49
    
I was making small modifications here and there to your original source code... Now it's kind of snowballed into a mini pet project. github.com/AydinAdn/Blackjack I've refactored so much of your code I'm not sure you'll even recognize it anymore but have a look at it ;). I'll expand on this post tomorrow (ill also post credits to GitHub tomorrow), it's 3AM here. –  Aydin Adn Aug 19 at 2:20
1  
@RubberDuck, there are single deck Blackjack games still being played in Deadwood,SD at Saloon #10 I think it might be the only single deck game in Deadwood. (Last Time I was there to Play Blackjack) –  Malachi Aug 21 at 15:09

3 Answers 3

Is unshuffled deck ever useful for anything? If not, you might want to call Shuffle() at the end of Initialize(), which will also avoid having to call Shuffle() after Initialize(), which happens several times in your code.


cards.Add(new Card() { Suit = (Suit)i, Face = (Face)j });

if (j <= 8)
    cards[cards.Count - 1].Value = j + 1;
else 
    cards[cards.Count - 1].Value = 10;

You should use the Card object initializer to set Value too. Using Math.Min(), that could look like this:

cards.Add(new Card { Suit = (Suit)i, Face = (Face)j, Value = Math.Min(j + 1, 10) });

Card cardToReturn = cards[cards.Count - 1];
cards.RemoveAt(cards.Count - 1);
return cardToReturn;

This looks like you don't want List, you want Stack. With that, all this code would simplify to:

return cards.Pop();

public int GetAmountOfRemainingCrads()

Very simple Get methods are usually better as properties.


static void Main(string[] args)

When you're not using args, just remove that parameter:

static void Main()

ConsoleKeyInfo userInput = Console.ReadKey(true);

When you're not using the output of a function, you don't need to declare a variable for it:

Console.ReadKey(true);

Console.ReadLine().Trim().Replace(" ","")

What's the logic behind this? Why are you accepting strings like 10 0? And if you did intend that, what's the reason for that Trim()? Replace() will already remove all spaces, including those at the start and end of the string and I don't think any other whitespace characters (which is what the parameterless overload of Trim() removes) make sense here.


foreach (Card card in userHand)
{
    if (card.Face == Face.Ace)
    {
        card.Value += 10;
        break;
    }
}

I don't think your logic for handling aces is sufficient. If I understand the rules correcty, even an ace that's not among the first two cards can be counted as 11 and even ace that is among the first two can be counted as 1.


I thought I could really use a Hand class in the dealHand method.

I agree, there is enough repeated code to warrant a separate class for this.


bool insurance = false; ;

What does the second semicolon do here? Get rid of it.


//chips -= betAmount / 2;

Don't leave commented out code from previous versions of your code behind. After you know the commented out code is no longer useful, remove it completely.


if (userHand[0].Value + userHand[1].Value == 21 && insurance)
{
    amountLost = betAmount / 2;
    chips -= betAmount / 2;
}
else if (userHand[0].Value + userHand[1].Value != 21 && !insurance)
{
    amountLost = betAmount + betAmount / 2;
    chips -= betAmount + betAmount / 2;
}

You're already computing amountLost to display it, you should also use it to modify chips, since it will make your code more DRY:

if (userHand[0].Value + userHand[1].Value == 21 && insurance)
{
    amountLost = betAmount / 2;
}
else if (userHand[0].Value + userHand[1].Value != 21 && !insurance)
{
    amountLost = betAmount + betAmount / 2;
}

chips -= amountLost;

foreach (Card card in userHand)
{
    totalCardsValue += card.Value;
}

This could be simplified using LINQ:

int totalCardsValue = userHand.Sum(card => card.Value);

default:
    break;

This is like adding else { } after an if; it doesn't do anything, remove it.


Console.ReadLine();

Why is this here? How is the user supposed to know they have to press Enter here?

share|improve this answer
    
thanks so much!, i'm gonna read it when I wake up –  user3626517 Aug 17 at 22:15

Override ToString() for

  • Card
  • Deck
  • Hand

Both Deck and Hand overrides should take advantage of Card.ToString().

PrintDeck() should be this dead simple:

public void PrintDeck() { Console.WriteLine(deck.ToString()); }

What, you want the cards numbered? OK. PrintDeck(), as shown, will not change.


public Card DrawACard()
{
    if (cards.Count <= 0)
    {
       this.Initialize();
       this.Shuffle();
    }
  • Re-initializing the deck in the middle of a game?
  • The Deck class has no business acting like the dealer. Sure, Deck should know how many cards it has, but if there are no cards left why are we trying to fetch one in the first place?
  • The game/dealer should keep track of when to re-initialize the deck. You already have some logic for that.
  • A Deck of cards is not a blackjack game. Don't put game logic/control in Deck.
share|improve this answer
    
The problem with ToString is that it often gets overloaded for many different purposes. I would argue that it's better to create an interface that has a DisplayName property perhaps. Other than that I upvoted your answer because of the other points you made. –  craftworkgames Aug 18 at 23:30

Your Card class has a potential problem, imagine such an initialization

new Card(){Suit = Suit.Heart, Face = Face.Seven, Value = 139213};

I believe you understand the problem, there is no relation between your Face and Value. You should have a constructor for your Card class that would only take Suit and Face as parameters and you should calculate the Value. You could use a private static Dictionary<Face,int> _valuesForFaces that could give you values for faces (Apologies if I miss something important, I know nothing about BlackJack). In this example I initialize the dictionary in the static constructor, which is called only once.

public class Card
{
    private static readonly Dictionary<Face,int> _valuesForFaces;

    public Suit Suit { get; private set; }
    public Face Face { get; private set; }
    public int Value { get; private set; }

    static Card()
    {
        _valuesForFaces.Add(Face.Ace, 1); //etc..
    }

    public Card(Suit suit, Face face)
    {
        Suit = suit;
        Face = face;
        Value = _valuesForFaces[face];
    }
}

In your Deck.Initialize method, you use the numbers 4 and 13 in your for loops, 8 in a if. This is a common anti-pattern named the Magic Number. Reading this I (who knows nothing about BlackJack) cannot understand what your code does, why is it 13 and not 27? (It is understandable after reading the code that it is the faces and suits, but still!) You should put them in well named constants so that it is easier to debug/read for any programmers. Plus your Initialize method shouldn't do the job of getting the card value, leave that to the Card class (as I shown above!).

In the PrintDeck method, you use a foreach with an index, maybe it's just a matter of opinion but why not use a for?

for(int i = 0;i < cards.Count;i++)
{
    Card card = cards[i];
    Console.WriteLine("Card {0}: {1} of {2}. Value: {3}", i+1, card.Face, card.Suit, card.Value);
}

In the DealHand method there is another magic number, I suggest you put it in a constant too. I didn't have time to check the rest of the code, I hope these suggestions will help you out!

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.