10
\$\begingroup\$

I'm a C# beginner and this is my try at making a command line Blackjack. I'm aware that the Dealer logic is still far from done, but I wanted to post my code for review.

I want to make the code as efficient as possible. Also, I want it to be readable, so that working with it after pauses is easier. I think it's quite readable already, but I know some of you can give me hints to make it better.

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

namespace CliBlackjackV2
{
    class Program
    {

        private static Random randomizer = new Random();
        private static bool gameLoop = true;
        private static bool cardDraw = true;


        enum cardColor
        {
            Spades,
            Hearths,
            Clubs,
            Diamonds,
        };

        enum cardFace
        {
            Ace,
            Two,
            Three,
            Four,
            Five,
            Six,
            Seven,
            Eight,
            Nine,
            Ten,
            Jack,
            Quen,
            King,
        };

        static void Main(string[] args)
        {
            while (gameLoop)
            {
                game();
                Console.Write("\n Play again? [Y/N]: ");
                string choice = Console.ReadLine().ToLower();
                switch (choice)
                {
                    case "n":
                        gameLoop = false;
                        break;
                    default:
                        break;
                }
            }
        }

        static void game()
        {
            //isl is the shortform for integer string list
            //since im treating the Faces as ints and the Collors as String (at least in my head)
            List<KeyValuePair<cardFace, cardColor>> islPlayerHand = new List<KeyValuePair<cardFace, cardColor>>();
            List<KeyValuePair<cardFace, cardColor>> islDealerHand = new List<KeyValuePair<cardFace, cardColor>>();

            islPlayerHand = getCards(islPlayerHand);
            islPlayerHand = getCards(islPlayerHand);

            islDealerHand = getCards(islDealerHand);
            islDealerHand = getCards(islDealerHand);

            renderBoard(islPlayerHand, islDealerHand);

            cardDraw = true;
            while (cardDraw)
            {
                cardDraw = playerCardDraw(islPlayerHand);
                renderBoard(islPlayerHand, islDealerHand);
                checkHand("Player", islPlayerHand);
            }

            while (true)
            {
                int dealerTotal = calcHandTotal(islDealerHand);
                while (dealerTotal < 16)
                {
                    islDealerHand = getCards(islDealerHand);
                    renderBoard(islPlayerHand, islDealerHand);
                    checkHand("dealer", islDealerHand);
                    dealerTotal = calcHandTotal(islDealerHand);
                }
                if (dealerTotal >= 16)
                {
                    break;
                }

            }

            renderBoard(islPlayerHand, islDealerHand);
            checkHand("Player", islPlayerHand);
            checkHand("dealer", islDealerHand);
            decideWinner(islPlayerHand, islDealerHand);
        }

        static void checkHand(string player, List<KeyValuePair<cardFace, cardColor>> hand)
        {
            int total = calcHandTotal(hand);

            if (total == 21)
            {
                Console.WriteLine($"\n {player} got a Blackjack!\n");
                cardDraw = false;
            }
            else if (total > 21)
            {
                Console.WriteLine($"\n {player} Busted!\n");
                cardDraw = false;

            }
        }

        static bool playerCardDraw(List<KeyValuePair<cardFace, cardColor>> playerHand)
        {
            bool cardDraw = true;
            Console.Write(" Would you like to (H)it or to (S)tand: ");
            string choice = Console.ReadLine().ToLower();

            switch (choice)
            {
                case "h":
                    playerHand = getCards(playerHand);
                    break;

                case "s":
                    cardDraw = false;
                    break;

                default:
                    break;
            }
            return cardDraw;
        }

        static int calcHandTotal(List<KeyValuePair<cardFace, cardColor>> hand)
        {
            int handTotal = 0;
            int aceCount = 0;

            foreach (KeyValuePair<cardFace, cardColor> card in hand)
            {

                if ((int)card.Key >= 10)
                {
                    handTotal += 10;
                }
                else if (card.Key == cardFace.Ace)
                {
                    aceCount++;
                }
                else
                {
                    handTotal += (int)card.Key + 1;
                }

            }

            //calculating the ace's Values properly
            if (aceCount > 0)
            {
                if (handTotal <= (11 - aceCount))
                {
                    handTotal += (aceCount - 1) + 11;
                }
                else
                {
                    handTotal += aceCount;
                }
            }
            return handTotal;
        }

        static void decideWinner(List<KeyValuePair<cardFace, cardColor>> playerHand, List<KeyValuePair<cardFace, cardColor>> dealerHand)
        {
            int hand1Total=calcHandTotal(playerHand);
            int hand2Total=calcHandTotal(dealerHand);

            if ((hand1Total > 21) && (hand2Total > 21))
            {
                Console.WriteLine(" Both busted!");
            }
            else if (hand1Total > 21)
            {
                Console.WriteLine(" Dealer Wins!");
            }
            else if (hand2Total > 21)
            {
                Console.WriteLine(" Player Wins!");
            }
            else if (hand1Total > hand2Total)
            {
                Console.WriteLine(" Player Wins!");
            }
            else if (hand2Total > hand1Total)
            {
                Console.WriteLine(" Dealer Wins!");
            }
            else
            {
                Console.WriteLine(" It's a tie!");
            }
        }

        static void renderBoard(List<KeyValuePair<cardFace, cardColor>> playerHand, List<KeyValuePair<cardFace, cardColor>> dealerHand)
        {
            Console.Clear();
            Console.WriteLine("***********************\n" +
                "*      PlayerHand     *\n" +
                "***********************\n");
            printCards(playerHand);
            Console.WriteLine($" Total: {calcHandTotal(playerHand)}\n");

            Console.WriteLine("***********************\n" +
                "*      DealerHand     *\n" +
                "***********************\n");
            printCards(dealerHand);
            Console.WriteLine($" Total: {calcHandTotal(dealerHand)}\n");
        }

        static void printCards(List<KeyValuePair<cardFace, cardColor>> hand)
        {
            int cardCount = 1;
            foreach (var card in hand)
            {
                Console.WriteLine($" #{cardCount}  {card.Key} of {card.Value}");
                cardCount++;
            }
            Console.WriteLine("");
        }

        static List<KeyValuePair<cardFace, cardColor>> getCards(List<KeyValuePair<cardFace, cardColor>> hand)
        {
            hand.Add(new KeyValuePair<cardFace, cardColor>((cardFace)randomizer.Next(0, 12), (cardColor)randomizer.Next(0, 3)));
            return hand;
        }
    }
}
\$\endgroup\$
10
\$\begingroup\$

Instead of

switch (choice)
{
    case "n":
        gameLoop = false;
        break;
    default:
        break;
}

you could use something like the following:

gameLoop = choice != "n";

or if you want to ignore cases (which makes sense for this case):

gameLoop = choice.ToUpper() != "N";
// or
gameLoop = !choice.Equals("n", StringComparison.InvariantCultureIgnoreCase)

//isl is the shortform for integer string list
//since im treating the Faces as ints and the Collors as String (at least in my head)
List<KeyValuePair<cardFace, cardColor>> islPlayerHand = new List<KeyValuePair<cardFace, cardColor>>();
List<KeyValuePair<cardFace, cardColor>> islDealerHand = new List<KeyValuePair<cardFace, cardColor>>();
  • Consider using another struct / class for combining cardFace and cardColor instead of using a key value pair.
  • Do not use any abbreviations that have to be explained with comments.

e.g.

struct Card
{
    public cardFace Face {get; set; }
    public cardColor Color {get; set; }
}

List<Card> cardsPlayer = new List<Card>();
List<Card> cardsDealer = new List<Card>();

Instead of using procedural programming, you could structure your code more object oriented.

A first step could be, to encapsulate the "game code" in it's own 'Game' class. Than you could define the cards for the dealer / player as instance variables and define methods that work on these instance variables instead of passing it to (nearly) all methods.

After that, you could try to create a Player class that contains all the player related code instead of using a list of cards for the player.

The resulting code becomes more readable and maintainable because code that belongs together is encapsulated in class and will be hidden by other parts of the program.

The usage of your new structured code could look like:

var game = new Game();
var game.Start();
// or

var player = new Player("Payer's name");
player.DrawCard();
player.DrawCard();
player.CheckHand();
// ...
\$\endgroup\$
  • \$\begingroup\$ Thanks for the Reply! Yeah i should have planed it out more instead of just going for it and adding stuff while i go. I had already thought about classes but i was not sure how to use them properly. So i went without them in this Version. I also noticed that my approach of the list of cards with the KVP isn't ideal. I tunnel vision too much since im new, so i tried to make use of what i learned. The struct looks much cleaner and i want to write my code like that ! \$\endgroup\$ – Seitrox Apr 27 '17 at 9:22
  • \$\begingroup\$ I'm curious about your improved implementation if you like to post it again ;) \$\endgroup\$ – JanDotNet Apr 27 '17 at 11:50
  • \$\begingroup\$ I will post the improved version. But i wana give it a bit time. My plan is doing it at the Weekend :D \$\endgroup\$ – Seitrox Apr 27 '17 at 13:20
5
\$\begingroup\$

Don't put all of your code in Program.cs. IMHO that file's Main method should basically do one thing: call a method in a class you wrote yourself.


Follow naming guidelines. Classes, enum names, methods should be PascalCase, for instance.


Don't use a Hungarian notation, especially not one of your own invention:

isl is the shortform for integer string list

Your IDE should tell you what type a variable is, not its name.


If there are strings you use repeatedly, e.g. "Player" and "dealer", make them const.

\$\endgroup\$
  • \$\begingroup\$ I didn't know that i used a bad notation. If PascalCase should be used for methods, what should i use for the rest? I knew bevore i started to write the programm that it is bad doing all in the program.cs (i should have stated that in the initial post!), I did it anyways so the difference to the next and final versions would be more significant. Thanks for your advice! \$\endgroup\$ – Seitrox Apr 27 '17 at 13:47
  • 2
    \$\begingroup\$ Here's the convention I like to use. Parameters and local variables always start with a lowercase letter. Private class members start with an underscore. Everything else starts with an uppercase letter. The thing I like most about this convention is that it allows me to see at a glance whether any given identifier is a local variable or a class member (because local variables always start with a lowercase letter and class members never start with a lowercase letter). \$\endgroup\$ – Tanner Swett Apr 27 '17 at 16:00
  • 1
    \$\begingroup\$ @TannerSwett Not only your convention, but this is the standard recommended by Microsoft too, and also by analysis tools by default. \$\endgroup\$ – Kroltan Apr 27 '17 at 17:40
5
\$\begingroup\$
while (true)
{
    int dealerTotal = calcHandTotal(islDealerHand);
    while (dealerTotal < 16)
    {
        islDealerHand = getCards(islDealerHand);
        renderBoard(islPlayerHand, islDealerHand);
        checkHand("dealer", islDealerHand);
        dealerTotal = calcHandTotal(islDealerHand);
    }
    if (dealerTotal >= 16)
    {
        break;
    }
}

can be rewritten as just

int dealerTotal = calcHandTotal(islDealerHand);
while (dealerTotal < 16)
{
    islDealerHand = getCards(islDealerHand);
    renderBoard(islPlayerHand, islDealerHand);
    checkHand("dealer", islDealerHand);
    dealerTotal = calcHandTotal(islDealerHand);
}

(that is, removing the outer while-loop and the if-statement)

This is because after the inner loop exits, you know that dealerTotal < 16 must not be true (otherwise, the inner loop would keep going), and thus dealerTotal >= 16 is true. So the body of the if-statement will be run in every iteration of the outer loop. Thus, the first iteration of the outer loop will result in the break; statement executing, meaning that the outer loop will only ever run once.

\$\endgroup\$
  • \$\begingroup\$ Thanks for pointing it out. I forgot that i could do that with the while. Kinda embarrassing :x \$\endgroup\$ – Seitrox Apr 27 '17 at 13:32
3
\$\begingroup\$

The terms are suit and rank. Should have a card class. You do too much logic about a card in the flow code. Should not calcHandTotal(). Hand should just have a public property for the total. Random cards on the fly is bad. You could have duplicate cards in a hand. Need a deck and a proper shuffle. Player class with name and hand so you can add in bet later.

\$\endgroup\$
  • \$\begingroup\$ I already thought about proper shuffling. I like your ideas and i will implement them in my improved version. At first i just wanted the basic thing to work which it does. This is meant for learning and all of the Answers are a great help for me. Thanks for youre time ! \$\endgroup\$ – Seitrox Apr 27 '17 at 13:23
2
\$\begingroup\$

In decideWinner(), why not name the variables playerHandTotal and dealerHandTotal, following the very descriptive convention of its arguments, instead of using the confusing hand1Total and hand2Total?

\$\endgroup\$
  • \$\begingroup\$ You are right, I will keep that in mind. In this version it doesnt make sense to name them hand1Total. I was thinking about adding more players later on and just set it to hand1 and hand2 total. But when i think about it now that doesnt even make much sense then. \$\endgroup\$ – Seitrox Apr 27 '17 at 13:31

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.