2
\$\begingroup\$

This is my revised command line Blackjack code. I tried to use as many suggestions as possible and ended up with this code. I also uploaded the project to GitHub if someone is interested.

The Dealer is now a bit smarter. The game now uses a card stack of 5 decks (which is easy to change), which is shuffled properly.

I'm not sure if I succeeded in using the naming convention that was spoken about in my last Post.

In the final version I want to implement a Betting system and the possibility for more players.

I hope to get suggestions on better class structuring/using, as well as clean coding (I think the _DecideWinner function in Game.cs can be done much cleaner and better).

Programm.cs

namespace CliBlackjack
{
    class Program
    {
        static void Main(string[] args)
        {
            Game game = new Game();
            while (game.moreGames)
            {
                game.PlayGame();
            }
        }
    }
}

Card.cs

namespace CliBlackjack
{
    enum Suit
    {
        Spades,
        Hearths,
        Clubs,
        Diamonds,
    };

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

    class Card
    {
        public Rank rank;
        public Suit suit;

        public Card(int Rank, int Suit)
        {
            rank = (Rank)Rank;
            suit = (Suit)Suit;
        }
    }
}

Player.cs

using System.Collections.Generic;

namespace CliBlackjack
{
    class Player
    {
        public int handTotal=0;
        public string name;
        public bool draw=true;
        public bool Busted = false;
        public List<Card> hand = new List<Card>();

        public Player(string Name)
        {
            name = Name;
        }

        public void GetCards(List<Card> Deck)
        {
            hand.Add(Deck[0]);
            Deck.RemoveAt(0);
            _CalcHandTotal();
        }

        private void _CalcHandTotal()
        {
            handTotal = 0;
            int aceCount = 0;
            foreach (var card in hand)
            {
                if (card.rank == 0)
                {
                    aceCount++;
                }
                else if ((int)card.rank < 10)
                {
                    handTotal += (int)card.rank+1;
                }
                else
                {
                    handTotal += 10;
                }

            }

            if (aceCount > 0)
            {
                if (handTotal <= (11 - aceCount))
                {
                    handTotal += (aceCount - 1) + 11;
                }
                else
                {
                    handTotal += aceCount;
                }
            }

        }

    }
}

Deck.cs

using System;
using System.Collections.Generic;
using System.Linq;

namespace CliBlackjack
{
    class Deck
    {
        private int _deckCount;
        public List<Card> deck = new List<Card>();
        Random rnd = new Random();

        public void CreateDeck(int DeckCount)
        {
            //removes the remaining Contents of the deck, once a new deck is needed
            deck.Clear();

            _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++)
                {
                    //Itterate over Card Suites
                    for (int z = 0; z < 4; z++)
                    {
                        deck.Add(new Card(x, z));
                    }
                }
            }
        }

        public void Shuffle(int TimesToShuffle)
        {
            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;
                }
            }
        }
    }
}

Game.cs

using System;

namespace CliBlackjack
{
    class Game
    {
       public bool moreGames = true;

       public void PlayGame()
        {
            string choice="";
            Console.Clear();
            Player player1 = new Player("Player");
            Player dealer = new Player("Dealer");

            Deck gameDeck = new Deck();

            gameDeck.CreateDeck(5);
            gameDeck.Shuffle(10);


            _GetStartingHand(player1, gameDeck);
            _GetStartingHand(dealer, gameDeck);

            _DisplayHand(dealer);
            _DisplayHand(player1);

            _CheckHand(player1);
            _CheckHand(dealer);

            if (!(player1.Busted || dealer.Busted))
            {
                _DrawPhase(player1, dealer, gameDeck);
            }

            _DecideWinner(player1, dealer);

            Console.Write("\n Play again? (Y/N): ");
            choice = Console.ReadLine();



            moreGames = choice.ToUpper() != "N";




        }

        //this doesnt seem to be the best way to decide the winner, works for now.
        private void _DecideWinner(Player Player, Player Dealer)
        {
            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!");
            }
        }

        private void _DrawPhase(Player Player, Player Dealer, Deck Deck)
        {

            while (Player.draw)
            {

                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))
            {
                Dealer.draw = Dealer.handTotal < 16;
                while (Dealer.draw)
                {

                    _CardDraw(Dealer, Deck);
                    _DisplayBoard(Player, Dealer);
                    _CheckHand(Dealer);

                    Dealer.draw = Dealer.handTotal < 16;
                }
                Console.Clear();
                _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)
        {
            Console.Clear();
            Player.GetCards(Deck.deck);
        }

        private void _CheckHand(Player Player)
        {
            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;
            }
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

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!

\$\endgroup\$
  • 1
    \$\begingroup\$ This Answer is Mindblowing. Wow, I never expected to get such an indepth review of my code. As of writing the comment I am halv way through it and will finish it later and probably re read it once i get back to improve the version (which will likely be tomorrow morning). The only thing I wanted to mention is that im not a native English speaker so naming and transaltion is messed up as you noticed and pointed out. I already find it incredibly helpfull that you explained what one could think and how to go about it. I would upvote that Answer more if I could !! \$\endgroup\$ – Seitrox Apr 29 '17 at 19:59
3
\$\begingroup\$

Deck.RemoveAt(0); is O(n) which is wasteful. Just keep an index. Player should not be manipulating Deck directly. Deck should be in control of Deck.

For every hand you start a new game with new players, a new 5 deck shoe, and shuffle it 10 times. Maybe a little excessive. In real life the table would play bit slow.

If you have a proper shuffle there is no purpose to multiple shuffles.

Use proper properties with get;

Why the excessive blank lines? Up to 5 blank lines!

Decide winner has a lot more decisions than it needs to. I don't think you have all the rules correct.

Really big. You are supposed to see the dealers up card.

\$\endgroup\$
  • \$\begingroup\$ For the new deck creation. Im aware of that. I was going to implement the game loop better when i add the bets. Yeah i don't know how to do decidewinner it better yet. I was going to have a new deck created once halv of the cards were used. While writing the comment i noticed how the index could help with that so i don't have to delete the cards. Thanks again for your time ! \$\endgroup\$ – Seitrox Apr 29 '17 at 13:00
  • \$\begingroup\$ "If you have a proper shuffle there is no purpose to multiple shuffles." there was a time when it was believed that "shuffling the deck" left patterns in the deck that could be exploited and if one wanted to model to prevent that then shuffling the deck ( optional random) number of times would resolve it. \$\endgroup\$ – Enigma Maitreya Apr 29 '17 at 19:23
  • \$\begingroup\$ @EnigmaMaitreya Do you have a citation on that? Do you know what a Fisher Yates shuffle is? \$\endgroup\$ – paparazzo Apr 29 '17 at 19:26
  • \$\begingroup\$ Were I am coming from is more back in the early 90's and modeling specific casino's for a means of improving the odds. I probably shouldn't have even brought it up as the way they do it now takes care of / resolves the issue. Yes the distinction would be Algorithmic Shuffle in a computer vs Mechanical/Hand shuffle. \$\endgroup\$ – Enigma Maitreya Apr 29 '17 at 19:35
  • \$\begingroup\$ @Paparazzi i just saw you edited your answer. With the dealers up card. I know about that but i have seen a lot of styles. And it wouldn't be hard to just display one of the dealers cards in this version. I just didnt feel it was necesarry to do it just yet. And the blank lines. I sometimes delete stuff and forget to also delete the blank lines with it. Is that realy that big of a deal ? \$\endgroup\$ – Seitrox Apr 29 '17 at 19:36
2
\$\begingroup\$

This piece of code in Player relies on intimate details of the implementation of Card, in particular, the (int) casts:

handTotal = 0;
int aceCount = 0;
foreach (var card in hand)
{
    if (card.rank == 0)
    {
        aceCount++;
    }
    else if ((int)card.rank < 10)
    {
        handTotal += (int)card.rank+1;
    }
    else
    {
        handTotal += 10;
    }
}

It would be better if Card encapsulated its implementation details (that you can case to int to get value), and exposed public methods to calculate values.

Rewritten that way, the above code snippet should look more like this:

handTotal = 0;
int aceCount = 0;
foreach (var card in hand)
{
    if (card.IsAce())
    {
        aceCount++;
    }
    else
    {
        handTotal += card.Value();
    }
}

This way Card has a natural public API, and its implementation details are well-hidden.

\$\endgroup\$
  • \$\begingroup\$ Makes a lot of sense. Thanks for showing me another, better way to do it! \$\endgroup\$ – Seitrox Apr 29 '17 at 18:49
  • \$\begingroup\$ @Seitrox I told you this on your first post \$\endgroup\$ – paparazzo Apr 29 '17 at 19:00
  • \$\begingroup\$ @Paparazzi that's true. But as a beginner i just couldn't get my head around it. You didn't provide an example like he did. So my mind went on and made this version ^^. \$\endgroup\$ – Seitrox Apr 29 '17 at 19:11
  • \$\begingroup\$ @Seitrox Cool you seem to be making an effort \$\endgroup\$ – paparazzo Apr 29 '17 at 19:13

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.