A command-line Blackjack is both a good way to learn the basics and to practice programming (a.k.a. a "kata"). Good effort on this! I took a look through it and added comments as I went:
namespace CliBlackjack { class Program { static void Main(string[] args) { Game game = new Game();
There's only ever one game, so you could merge the contents of Game into Program. Doing so would make the code less complex because readers of this part wouldn't need to jump to another file to find out what moreGames and PlayGame do. It would also tie the game to this command line program. That's a tradeoff, but you should consider choosing the simpler path for now and the more complex route later if and and when it's deemed necessary
while (game.moreGames)
What does it mean for a game to have more games? Isn't a game just a single game? Having read the code I know the answer to this question, but readers like me will be initially confused at what that means. You might want to consider making two explicit steps here: "play game" and "play again prompt".
{ game.PlayGame(); } } } } namespace CliBlackjack { enum Suit { Spades, Hearths,
Spelling: Hearths should be Hearts.
Clubs, Diamonds, }; enum Rank { Ace, Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Quen,
Spelling: Quen should be Queen.
King, }; class Card { public Rank rank; public Suit suit;
These are never changed, so consider making them readonly or auto-properties with a private set: public Rank rank { get; private set; }. Doing this will make the Card type immutable and it will therefore take on value semantics, which has various benefits. Alternatively, consider making Card a struct instead of a class. Since it consists of just two enum fields, copying it will be just as cheap as copying a class reference and it will automatically become a value type.
Also, this is not the normal naming convention for public fields and properties. That's fine, but you should make this choice knowing that you're not following the norms for C# code. The normal naming convention is PascalCase.
public Card(int Rank, int Suit) {
Card requires specific enumerated values for its rank and suit, but this constructor allows any int value. Instead, it should take the Rank and Suit enum types.
This is also not the naming convention for parameters in C# code. The normal naming convention is camelCase.
rank = (Rank)Rank; suit = (Suit)Suit; } } } using System.Collections.Generic; namespace CliBlackjack { class Player { public int handTotal=0;
This value needs to be kept in sync with the cards in hand. In this case that's pretty easy, but in general it's good to think about why you're caching this data that is quickly and easily derived by _CalcHandTotal. For example, you could remove this variable and make _CalcHandTotal a public function returning int instead of setting this variable. Yes, the hand total would need to be recalculated each time, but how many times is that? Two or three? How expensive is it to recalcuate? A few nanoseconds? Personally, I'd remove this field in favor of recalculating so I could gain the benefits of simpler code.
Also, there is no need to initialize handTotal to zero since that is already its default value.
public string name; public bool draw=true;
The draw field is only used as a control variable for _DrawPhase and _CheckHand, not by this class. Those functions should take care of this variable, rather than this class carrying one around for all possible users of this class.
public bool Busted = false;
This is also the default value for bool fields and unneccessary.
public List<Card> hand = new List<Card>(); public Player(string Name) { name = Name; } public void GetCards(List<Card> Deck)
This function name doesn't describe what it's doing with enough specificity. What cards is it getting? Actually, it only gets one card. It happens to get the first card, but there are three side-effects to that: it removes the first card from the deck, it adds that card to its hand, and it updates the handTotal field. Consider instead a well-known term that neatly describes what this function represents: Draw.
{ hand.Add(Deck[0]); Deck.RemoveAt(0);
As mentioned in the review by @Paparazzi, this is quite inefficient. Internally, the List class holds an array of Card. When you remove at the first index it needs to copy the card at the second index to the first index. It then repeats this over and over until all of the cards have shifted toward the beginning by one element. For a deck of just a few dozen or hundred cards (since you use five decks), that's still going to be very fast. In general though it's much more efficient to operate on the end of the list. If you remove from the end of the list then there's no swapping to do at all. The list is shuffled, so it shouldn't matter which end of it you consider the "top" or "bottom".
Once you've shifted to removing from the top, it's natural to switch Deck from a List to a Stack. After all, the deck is literally a stack of cards. You'll get some more intuitive naming out of it (e.g. Pop) and it may be implemented more efficiently than the equivalent List operations.
_CalcHandTotal(); } private void _CalcHandTotal()
As described above, consider making this public and returning int rather than setting handTotal.
{ handTotal = 0; int aceCount = 0; foreach (var card in hand) { if (card.rank == 0)
Card.rank is a Rank, so you should compare with Rank.Ace instead of 0. That will make this line more readable because it removes the need to mentally map 0 to Rank.Ace. It's a minor inconvenience, but there's little reason to use the integer value here.
{ aceCount++; } else if ((int)card.rank < 10)
Similar to above, consider using Rank.Jack instead of 10.
{ handTotal += (int)card.rank+1; } else { handTotal += 10; } } if (aceCount > 0) { if (handTotal <= (11 - aceCount)) { handTotal += (aceCount - 1) + 11; } else { handTotal += aceCount; } } } } } namespace CliBlackjack { class Deck { private int _deckCount;
This field is never used and can be removed.
public List<Card> deck = new List<Card>();
As mentioned before in Player._CalcHandTotal, consider using a Stack instead. Also, deck is never set after the Deck constructor so it can be either readonly or an auto-property with a private set.
Random rnd = new Random();
rnd is only used by Shuffle and can be moved there as a local variable. Alternatively, it can be a parameter to Shuffle if you want to reuse the same Random object repeatedly.
Note that by removing these two fields the Deck class has only one left: deck. At this point you should consider removing the Deck class and converting its two functions to take the deck as parameter.
public void CreateDeck(int DeckCount)
I find the name CreateDeck puzzling in this case. Can decks create other decks? Why isn't a Deck returned then? Where is this created deck? I have to have a Deck to call this function, so don't I already have the deck? Also, what does DeckCount mean? It seems to indicate how many decks I want to create, but I have no idea where those decks are being created.
Of course I've read the code so I understand what is happening, but in general this could benefit from some more accurate naming. Instead of CreateDeck, consider AddCards as that makes it clearer that you're filling an existing deck with cards rather than making a new one. As for DeckCount, the ambiguity lies in the conceptual meaning of "deck." In the case of Deck you use it as a collection of cards but in the case of DeckCount you use it as a specific collection of 52 cards consisting of the usual 13 ranks and 4 suits. I think the latter use is more common, but I could be wrong. In any case, consider renaming one of them. For example, Deck could be renamed DrawPile.
{ //removes the remaining Contents of the deck, once a new deck is needed deck.Clear();
This function is always called on a newly-created Deck, so there is no need to clear the list.
_deckCount = DeckCount; //Itterate for the given amount of Decks for (int i = 0; i < _deckCount; i++) { //Itterate over Card Ranks for (int x = 0; x < 13; x++)
The constant 13 happens to be the number of ranks in the Rank enum, but this is more mental mapping that the reader needs to do. It may also change if you add a joker rank, for example. Instead, you can add a new NumRanks to the end of Rank and it will automatically be set right. Then you can replace 13 with Rank.NumRanks.
{ //Itterate over Card Suites for (int z = 0; z < 4; z++)
Similar to above, consider adding Suit.NumSuits.
{ deck.Add(new Card(x, z)); } } } } public void Shuffle(int TimesToShuffle)
There is probably no need to shuffle more than once.
{ for (int i = 0; i < TimesToShuffle; i++) { //Fisher Yates Shuffle for (int cardIndex1 = deck.Count() - 1; cardIndex1 > 0; cardIndex1--) { int cardIndex2 = rnd.Next(cardIndex1 + 1); var tmp = deck[cardIndex1]; deck[cardIndex1] = deck[cardIndex2]; deck[cardIndex2] = tmp; } } } } } using System; namespace CliBlackjack { class Game { public bool moreGames = true; public void PlayGame() { string choice="";
Some programmers prefer using string.Empty, so it's worth considering. Also, prefer declaring variables as close to their usage as possible so there's less for readers to remember and less code to scrutinize to see how the variable is used. In this case choice isn't used until almost the end of this function.
Console.Clear();
Clearing the console is probably not something that the caller expects this function to do. Consider moving the Clear call out of this function.
Player player1 = new Player("Player");
There's only one player, so you might consider renaming this variable to just player.
Player dealer = new Player("Dealer"); Deck gameDeck = new Deck(); gameDeck.CreateDeck(5); gameDeck.Shuffle(10); _GetStartingHand(player1, gameDeck); _GetStartingHand(dealer, gameDeck);
More explicit naming could help here. The word "get" is pretty general, so consider "draw" to indicate the side-effect such as removing cards from the deck.
_DisplayHand(dealer); _DisplayHand(player1);
You have a _DisplayBoard` function that makes these two calls, so why not call it instead?
_CheckHand(player1); _CheckHand(dealer); if (!(player1.Busted || dealer.Busted)) {
Is it possible to bust on the starting hand? If not, you can remove this if.
_DrawPhase(player1, dealer, gameDeck);
_DrawPhase is only ever called from this one location, so you could move its contents here. By doing that you allow the reader of the code to continue reading linearly rather than jumping to a later position in the file, mapping parameters from here to there, and considering the interaction of that function with this one. It's much less complex and should be considered as an alternative.
} _DecideWinner(player1, dealer);
Similar to _DrawPhase, this function can also be moved in here.
Console.Write("\n Play again? (Y/N): "); choice = Console.ReadLine(); moreGames = choice.ToUpper() != "N";
Instead of setting a field, consider returning a bool. Doing so removes the only field of this class (moreGames) and therefore you can make this class and every function in it static. That's usually a good thing as it decreases complexity by eliminating a hidden this parameter to every function. Side-effects are much less likely and, as such, the code is more easily understood, parallelized, and so forth.
}
Some extra newlines are fine for aesthetics, but many would consider more than one or two newlines excessive. It's up to personal taste though.
//this doesnt seem to be the best way to decide the winner, works for now. private void _DecideWinner(Player Player, Player Dealer)
This function not only decides on the winner but acts on that decision via a side-effect: printing to the console. Callers might not expect that behavior and might want to do something else. In this case they don't, but it's often a good idea to decouple the decision from the action based on that decision. Here you could return an Outcome enum with PlayerWin, DealerWin, Tie, and BothBust values. Another function, or even the caller, could switch on that return value and perform an appropriate action such as printing to the console. There are other benefits of this approach, too. If you do this split then you'll realize this function doesn't need Player parameters since it only uses their handTotal. So you can make it safer and more general by taking int parameters.
{ if (Player.handTotal == 21 && Dealer.handTotal == 21) { Console.WriteLine("\n It's a tie!"); } else if (Player.handTotal == 21 && Dealer.handTotal != 21) { Console.WriteLine($"\n {Player.name} wins!"); } else if (Player.handTotal != 21 && Dealer.handTotal == 21) { Console.WriteLine($"\n {Dealer.name} wins!"); } else if (Player.handTotal > 21 && Dealer.handTotal <= 21) { Console.WriteLine($"\n {Dealer.name} wins!"); } else if (Player.handTotal <= 21 && Dealer.handTotal > 21) { Console.WriteLine($"\n {Player.name} wins!"); } else if (Player.handTotal > 21 && Dealer.handTotal > 21) { Console.WriteLine($"\n Both Busted "); } else if (Player.handTotal> Dealer.handTotal) { Console.WriteLine($"\n {Player.name} wins!"); } else if (Player.handTotal < Dealer.handTotal) { Console.WriteLine($"\n {Dealer.name} wins!"); } else if (Player.handTotal == Dealer.handTotal) { Console.WriteLine("\n It's a tie!"); }
There are only four cases: player wins, dealer wins, tie, both bust. As such the code should look like if (...) else if (...) else if (...) else. Arranging it like this will reduce the amount of duplicated code (Console.WriteLine calls in this case) and make it easier for readers to understand this function.
} private void _DrawPhase(Player Player, Player Dealer, Deck Deck) { while (Player.draw)
As mentioned before, the Player.draw field should be removed and turned into a local variable of this function. It's currently also set by _CheckHand, but you can get around that easily by having _CheckHand return a bool instead of setting Player.draw. Then simply have this function assign that return value to the new local variable.
Also, since _DrawPhase always begins with the player being eligible for drawing you can convert this function into a do-while loop.
{ Console.Write("\n Would you like to (H)it or (S)tand: "); string choice = Console.ReadLine(); Player.draw = choice.ToUpper() != "S"; if (Player.draw) { _CardDraw(Player, Deck); _DisplayBoard(Player, Dealer); _CheckHand(Player); } } if (!(Player.Busted))
You may prefer them for aethetic reasons, but these paretheses are unnecessary.
{ Dealer.draw = Dealer.handTotal < 16; while (Dealer.draw)
You can assign in a while loop and use that to avoid code duplication in this case. Instead, if you use while (Dealer.draw = Dealer.handTotal < 16) then you can remove the last line of this loop.
{ _CardDraw(Dealer, Deck); _DisplayBoard(Player, Dealer); _CheckHand(Dealer); Dealer.draw = Dealer.handTotal < 16; } Console.Clear();
As above, side-effects like clearing the console are usually unexpected.
_DisplayBoard(Player, Dealer); _CheckHand(Player); _CheckHand(Dealer); } } private void _DisplayBoard(Player Player, Player Dealer) { _DisplayHand(Dealer); _DisplayHand(Player); } private void _DisplayHand(Player Player) { Console.WriteLine("\n***********************\n" + $"* {Player.name}Hand *\n" + "***********************\n"); foreach (var card in Player.hand) { Console.WriteLine($" - {card.rank} of {card.suit}"); } Console.WriteLine($"\n Total: {Player.handTotal}"); } private void _GetStartingHand(Player Player, Deck Deck) { Player.GetCards(Deck.deck); Player.GetCards(Deck.deck); } private void _CardDraw(Player Player,Deck Deck)
Functions are normally named as verbs. In this case _DrawCard would be more conventional.
At this point I should also mention that it's rather unconventional to begin private function names with an underscore. That's a personal choice though and you certainly don't need to comply with anyone's rules if you don't want to. One bit of formatting that can be helpful is to be consistent though. For example, here you have no space after the comma in your parameters list but you do have spaces in other functions like _GetStartingHand.
{ Console.Clear();
As above, side-effects like clearing the console are usually unexpected.
Player.GetCards(Deck.deck); } private void _CheckHand(Player Player)
As mentioned above, consider returning bool instead of setting a field on the parameter (i.e. mutating it). Also mentioned above, this may be a good candidate for splitting the "check" from the "action" as this function currently mixes them. It's more like _CheckHandAndPrintIfGameOver than just _CheckHand. Returning a result enum of some sort (e.g. HandStatus with Ok, Busted, and Blackjack) allows the caller to decide how to act on that status.
{ if (Player.handTotal > 21) { Console.WriteLine($"\n {Player.name} Busted!"); Player.draw = false; Player.Busted = true; } else if (Player.handTotal == 21) { Console.WriteLine($"\n {Player.name} got a Blackjack!"); Player.draw = false; } } } }
Again, good job getting this working. I hope you'll find this review helpful!
