Another attempt at a Blackjack game

Question

This is a follow up question to this one.

I have been working on this game for some time, and now I would like it to be reviewed. Most importantly, I want to know about my code readability, commenting, naming, efficiency, code practicality, class and method splitting. Also I'm afraid that the code is needlessly complicated (but that could be because I still consider myself a beginner), if that is the case please mention that too in your review.

I consider the game to be finished, since it has all the features that I wanted to implement. I think the only thing missing is some sort of betting system that I might add in the future.

Program class:

using System;

class Program
{
    static void Main(string[] args)
    {
        //Variable declaration
        Console.Write("Insert a name: ");
        string playerName = Console.ReadLine();
        bool play = true;
        while (play == true)
        {
            Game game = new Game();
            game.Play(playerName);
            Console.WriteLine("Press ENTER to continue");
            Console.ReadLine();
            Console.Clear();
            //ask for replay
            Console.Write("Do you want to play again? yes/no: ");
            bool answer = true;
            string ans = Console.ReadLine();
            //Looping until a correct answer is given
            while (answer)
            {
                if (ans == "y" || ans == "Y" || ans == "yes" || ans == "YES")
                {
                    play = true;
                    answer = false;
                }
                else if (ans == "n" || ans == "N" || ans == "no" || ans == "NO")
                {
                    play = false;
                    answer = false;
                }
                else
                {
                    Console.Write("error, unexpected input\nPlease type y or n ");
                    ans = Console.ReadLine();
                }
            }
            Console.Clear();
        }
    }
}

Game Class:

using System;

enum Status
{
    gameOn,
    playerWin,
    houseWin,
    draw,
    blackjack
}

class Game
{
    byte status = (byte)Status.gameOn;
    House house = new House();
    Player player1 = new Player("");
    Card deck = new Card();

    //player hits as many times as he wants when < 21
    public void PlayerHit()
    {
        player1.CheckIfBust();
        if (!player1.Bust)
        {
            Console.Write("{0} Hits", player1.Name);
            player1.hand.Add(deck.DealCard(false));
            player1.GetTotal();
        }
    }

    //Initial cards
    public void InitialHits()
    {
        PlayerHit();
        PlayerHit();

        house.CheckIfBust();
        if (!house.Bust)
        {
            Console.Write("The House Hits");
            house.hand.Add(deck.DealCard(false));
            Console.WriteLine("The House gets a secret card");
            house.hand.Add(deck.DealCard(true));
            house.CheckIfBust();
            house.ChangeAces();
        }
    }

    //House hits untill it reaches 16
    public int HouseHit()
    {
        while (house.total < 16)
        {
            Console.Write("The House Hits");
            house.hand.Add(deck.DealCard(false));
            house.CheckIfBust();
            house.ChangeAces();
        }
        house.Stand();
        return house.total;
    }

    public void Play(string pName)
    {
        player1.Name = pName;
        player1.CheckIfBust();
        deck.CreateDeck();

        //Initial cards
        InitialHits();

        //Player wins automatically if he has a "blackjack"
        if (player1.HasBlackjack() == true)
        {
            Console.WriteLine($"{player1.Name} has a Blackjack!");
            status = (byte)Status.blackjack;
        }
        else
        {
            //loops as long as the player hasn't gone bust
            while (!player1.Bust && !player1.isStanding)
            {
                //Player gives input
                Console.WriteLine($"It is {player1.Name}'s turn:");
                Console.WriteLine("Press H to hit   Press S to stand" +
                    "\nPress V to view your hand");

                string input = Console.ReadLine();
                //input check
                if (input == "h" || input == "H")
                {
                    PlayerHit();
                }
                else if (input == "s" || input == "S")
                {
                    player1.Stand();
                    break;
                }
                else if (input == "v" || input == "V")
                {
                    player1.ViewHand();
                }
                else
                {
                    Console.Write("Unknown command\nPlease type a valid command ");
                    input = Console.ReadLine();
                }
                player1.CheckIfBust();
                player1.ChangeAces();
            }
            if (player1.Bust)
            {
                Console.WriteLine("{0}'s hand is {1} so he goes bust"
                    , player1.Name, player1.total);
            }
            HouseHit();
            CompareHands();
        }
        AnounceWinner();
    }

    public void AnounceWinner()
    {
        switch (status)
        {
            case 1:
                Console.WriteLine($"{player1.Name} wins!");
                break;
            case 2:
                Console.WriteLine("The House wins!");
                break;
            case 3:
                Console.WriteLine("Is is a draw");
                break;
            case 4:
                Console.WriteLine($"{player1.Name} wins!");
                break;
        }
    }

    void CompareHands()
    {
        if (player1.total > 21)
            player1.total = 0;
        if (house.total > 21)
            house.total = 0;

        if (player1.total > house.total)
        {
            status = (byte)Status.playerWin;
        }
        else if (house.total > player1.total)
        {
            status = (byte)Status.houseWin;
        }
        else
        {
            status = (byte)Status.draw;
        }
        if (player1.Bust && house.Bust)
        {
            status = (byte)Status.draw;
        }
    }
}

The Player class:

using System;
using System.Collections.Generic;

class Player
{
    public string Name { get; set; }
    public List<int> hand = new List<int>();
    public bool Bust = false;
    public bool isStanding = false;
    public int total;

    public Player(string name = "")
    {
        Name = name;
        total = 0;
    }

    public int GetTotal()
    {
        total = 0;
        foreach (int card in hand)
        {
            total += card;
        }
        return total;
    }

    public bool HasBlackjack()
    {
        return (total == 21) ? true : false;
    }

    public virtual void Stand()
    {
        CheckIfBust();
        if (!Bust)
        {
            Console.WriteLine("{0} stands at {1}", Name, total);
        }
        isStanding = true;
    }

    public virtual void ViewHand()
    {
        Console.WriteLine("{0}'s hand is {1}", Name, total);
    }

    public bool CheckIfBust()
    {
        GetTotal();
        return Bust = (total > 21) ? true : false;
    }

    public void ChangeAces()
    {
        while (Bust && hand.Contains(11))
        {
            hand[hand.FindIndex(index => index.Equals(11))] = 1;
            CheckIfBust();
        }
    }
}

The Dealer class:

using System;

class House : Player
{
    public override void Stand()
    {
        CheckIfBust();
        if (Bust)
        {
            Console.WriteLine("The Houses's hand was {0} so it went bust", total);
            total = 0;
        }
        else
        {
            Console.WriteLine("The House stands at {0}", total);
        }
    }
}

The Card class:

using System;
using System.Collections.Generic;

class Card
{
    private readonly Random randCard = new Random(DateTime.Now.Millisecond);
    public List<string> Deck = new List<string>();

    public void CreateDeck()
    {
        List<string> values = new List<string>
        {
            "2",
            "3",
            "4",
            "5",
            "6",
            "7",
            "8",
            "9",
            "10",
            "Jack",
            "Queen",
            "King",
            "Ace"
        };

        List<string> suits = new List<string>
        {
            "Spades",
            "Diamonds",
            "Hearts",
            "Clubs"
        };

        foreach (string value in values)
        {
            foreach (string suit in suits)
            {
                Deck.Add($"{value} of {suit}");
            }
        }
    }

    public int DealCard(bool secret)
    {
        int value = 0;
        int randomCard = randCard.Next(Deck.Count);
        string card = Deck[randomCard];
        Deck.RemoveAt(randomCard);

        if (!secret)
        {
            Console.Write(" and was dealt the {0}\n", card);
        }

        //Making the numbers exept 10 have their value
        if (card[0] == '2' || card[0] == '3' || card[0] == '4' || card[0] == '5' ||
            card[0] == '6' || card[0] == '7' || card[0] == '8' || card[0] == '9')
        {
            value = int.Parse(card[0].ToString());
        }
        //Making the faces and 10 have a value of 10
        else if (card[0] == '1' || card[0] == 'J' || card[0] == 'Q' || card[0] == 'K')
        {
            value = 10;
        }
        //Making the Aces have a value of 11
        else if (card[0] == 'A')
        {
            value = 11;
        }
        return value;
    }
}

Answer 1

Boolean used in conditions

Let's take the following example : if (skyIsBlue == true).

In a proper English sentence, we'd read :

If it is true that the sky is blue

But wait, we don't talk this way.

If the sky is blue

Well I'll be damned because it works the exact same way with programming languages. You can write : if(skyIsBlue) (or in your case while(play)).

Though, play hardly expresses a boolean value. I'd rename that. At first I thought about isPlaying, but that's not really what happens. We want to know if the player wants to start a new game. So I'd name it playerWantsToPlay (or you know, something like that).

Console inputs/outputs

Do you really need to ask the user to press Enter? My guess is no.

The following might be slightly advanced, but you'll learn something!

When using OOP like you did, you want your "domain" objects to be as independent as possible from the environment in which they're being used. In your case, it is very nearly well done. Though, you use the Console's methods in the Game class. There are multiple good ways to fix this but I think the best one would be to think like the blackjack game (lol).

The blackjack game doesn't communicate with you. It is observed. When you play blackjack, you are looking at the game. The game itself isn't telling you its state.

This sounds abstract and it is. To fix this problem, you need to shift some responsibilities in your code. The Game class should have methods to request a decision (pick a card or not) and methods to indicate its state.

For a quick example, I'd add the following methods to the Game class :

public void PlayTurn(Action action);
public int GetPlayerTotal();
public bool IsGameOver();
public ? GetWinner();

This is pretty much a draft, but I'm sure you get the idea. In Action you'd haveStandorHit`.

The goal of these "transformations" would be to shift the whole responsibility to interact with the Console from the Game class to the Main class.

Strings

Instead of :

ans == "y" || ans == "Y" || ans == "yes" || ans == "YES"

You could write :

string ans = Console.ReadLine().ToLower();
if(ans == "y" || ans == "yes")

Conventions

• Enum values should be PascalCased. So, for example, Status.GameOn.
• Don't use byte for nothing. It's just more "confusing" than anything else.
• Don't use public members. public bool Bust = false; should be private or a property. This might seem like an useless detail, but that's how encapsulation works and it's a good thing to get used to it.

Naming

1. ans vs answer

If I understood properly, ans is used to verify that an answer is valid. So isValidAnswer would be much more appropriate.

2. Use full names

What do you save by writing randCard instead of randomCard? Two characters, so basically nothing. It's not easier to understand or better to use short names.

3. Infamous magic "string"

A magic string is a string that is hardcoded somewhere that means something important that isn't described in the code. I'll also apply this to other variable types eg. player1.total > 21.

What does 21 mean? (I know, but what if someone who reads your code doesn't)

When I write a board/card game program, I'd like to think that someone who never played the game before could understand every rules by simply looking at my code. So I would introduce a variable that is properly named to represent the 21 value.

Data structures

• The whole Card values system could be replaced by a Dictionary<string,int>. Ex :

  cardValues["2"] = 10; ...

• Make your suite an enum.

Overall, I'd say you did a very good job for a beginner. I look forward to the third review iteration :)

Answer 2

Enums should be treated as enums and not numerically. @TopInFrassi touched on this too briefly when he said not to use byte. That doesn't mean you should use int for the enum either. Rather just use the enum as is, particularly with well chosen phrases as it makes your code more readable or less cryptic based on magic numbers.

Example

enum Status
{
    gameOn,
    playerWin,
    houseWin,
    draw,
    blackjack
}

Like TopinFrassi said, the convention is PascalCase. Change this to:

enum Status
{
    GameOn,
    PlayerWin,
    HouseWin,
    Draw,
    Blackjack
}

Later in Game class, this:

byte status = (byte)Status.gameOn;

Should be changed to:

Status status = Status.GameOn;

And the switch block in AnnounceWinner changes from:

public void AnounceWinner()
{
    switch (status)
    {
        case 1:
            Console.WriteLine($"{player1.Name} wins!");
            break;
        case 2:
            Console.WriteLine("The House wins!");
            break;
        case 3:
            Console.WriteLine("Is is a draw");
            break;
        case 4:
            Console.WriteLine($"{player1.Name} wins!");
            break;
    }
}

to a more self-documented:

public void AnounceWinner()
{
    switch (status)
    {
        case Status.PlayerWin:
            Console.WriteLine($"{player1.Name} wins!");
            break;
        case Status.HouseWin:
            Console.WriteLine("The House wins!");
            break;
        case Status.Draw:
            Console.WriteLine("Is is a draw");
            break;
        case Status.Blackjack:
            Console.WriteLine($"{player1.Name} wins!");
            break;
    }
}

Anyone trying to follow your code no longer has to figure out what 2 means.

Other

I would recommend specifying access modifiers everywhere, on classes, enums, methods, etc. Example:

public enum Stats { ... }

public class Game

Finally without getting into too many specifics, I'm not thrilled about the Card class as it does too much. Typically one would see a Rank and Suit enum, and then a Card would be an individual card containing a rank and a suit.

Put many cards together into a Deck class. An individual card cannot be shuffled. Rather a deck of cards is what is shuffled and dealt from. You can go even further that there is a standard deck of 52 cards (perhaps a static method), or you may be working with a deck consisting of 4 or more standard decks.

Answer 3

Your code relies heavily on methods being called in a particular order. For example, Bust is set to a meaningful value only after CheckIfBust has been called. Attempting to access Bust without having called the method would return an unreliable value—and there would be no warnings from the compiler. This extends to methods like ChangeAces—how can a user of the class know it’s necessary to call CheckIfBust first? Same for total/GetTotal. Instead use properties that calculate the correct values on the fly. Only for cases of great computational complexity should you store the values of such calculations in fields.

In general:

• Minimize the number of methods that change state. When you can just return a value, do so. Such methods can later be reordered as you wish without changing the overall behavior.
• Clearly label methods that do change state so it’s obvious what exactly is being changed. Then you can still edit your code and move things around if you watch the state-changing methods.
• Make dependencies explicit. If something must be called after some other thing, have the second thing call the first thing, or have the result of the first thing passed as a parameter to the second.

In real production code, requirements tend to change, code is edited often, and good code is resilient to various rewrites in surrounding code. This is helped by high cohesion and low coupling (look these up if unfamiliar). You have decent cohesion (would be improved by separating game logic from console output) and high coupling (improved by getting rid of aforementioned hidden dependencies).

Answer 4

I don't like calling the class Card when it is a deck.

If you and the dealer have blackjack I think you lose.

You can just assign the card rank 1 - 13

int points = rank;
if (point > 10) 
{
    points = 10;
}

Then code to deal with ace 1 or 11.

It does not need to be efficient but a new game each hand is not as efficient as one game with a shuffle between hands.

This is just silly:

return Bust = (total > 21) ? true : false;

If true return true. If false return false.

return Bust = total > 21;

External logic should not have to call ChangeAces. CheckIfBust should deal with (encapsulate) that.

Game should not be writing to the console. Should have a UI layer that consumes game.