8
\$\begingroup\$

I made this small Blackjack game in the past as a way to practice C++ basics and have fun at the same time. I stopped working on the game during the last two years of my computer science degree program and now in my free time I want to improve it. I'm looking for suggestions on how I can improve the game. Not just adding new features, but how I can improve the organization, eliminate unnecessary code, and stick to common (industry standard) programming practice. Feel free to make as many comments as you like! (I'm aware that I could add more comments)

Blackjack.cpp

#include "stdafx.h"
#include "Blackjack.h"


Blackjack::Blackjack()
{
    srand(time(0));
    dhandSize = 0;
    phandSize = 0;
    dhandSum = 0;
    phandSum = 0;
    playerDone = false;
    dealerDone = false;
}

void Blackjack::playGame()
{
    cout << "Welcome to Blackjack!\n";

    // Start the player and dealer with two cards
    addPlayerCard();
    addPlayerCard();
    addDealerCard();
    addDealerCard();
    sumHands();
    printHand();

    if (dhandSum == 21)
    {
        cout << "Dealer has blackjack. Dealer wins.\n";
        return;
    }
    else if (phandSum == 21)
    {
        cout << "Player has blackjack. Player wins.\n";
        return;
    }

    while (dealerDone == false || playerDone == false)
    {
        if (playerDone == false)
        {
            cout << "Would you like to hit? (1 - Yes, 2 - No)\n";
            cin >> phit;

            if (phit == 1)
            {
                addPlayerCard();
                printHand();
                sumHands();

                if (phandSum > 21)
                {
                    cout << "Player's hand exceeded 21. Player loses.\n";
                    return;
                }
            }
        }

        if (playerDone == false)
        {
            cout << "Would you like to stand? (1 - Yes, 2 - No)\n";
            cin >> pstand;
        }

        if (pstand == 1)
        {
            playerDone = true;
        }

        if (dhandSum < 17 && dealerDone != true)
        {
            addDealerCard();
            printHand();
            sumHands();

            if (dhandSum > 21)
            {
                cout << "Dealer hand exceeded 21. Dealer loses.\n";
                return;
            }
        }
        else if (dhandSum >= 17)
        {
            dealerDone = true;
        }

        if (phandSum == 21 && dhandSum == 21)
        {
            cout << "Push, player and dealer reached 21.\n";
            return;
        }
        else if (phandSum == 21)
        {
            cout << "Player reached 21. Player wins.\n";
            return;
        }
        else if (dhandSum == 21)
        {
            cout << "Dealer reached 21. Dealer wins.\n";
            return;
        }

        if ((playerDone == true && dealerDone == true) || (phandSize == 5 && phandSize == 5))
        {
            if (dhandSum < phandSum)
            {
                cout << "Sum of your hand exceeds the dealer's sum of " << dhandSum << "! You win!";
                return;
            }
            else if (phandSum == dhandSum)
            {
                cout << "Dealer sum of " << dhandSum << " is equal to the sum of your hand. Tie game.";
                return;
            }
            else if (dhandSum > phandSum)
            {
                cout << "Sum of your hand is lower than the dealer's sum of " << dhandSum << ". You lose!";
                return;
            }
        }
    }
}

int dhand[5];
int phand[5];
int dhandSize;
int phandSize;
int dhandSum;
int phandSum;
int phit;
int pstand;
bool playerDone;
bool dealerDone;

void Blackjack::addPlayerCard()
{
    if (phandSize <= 5)
    {
        phand[phandSize] = 1 + (rand() % 11);
        phandSize++;
    }
    else
    {
        cout << "Sorry. You have reached the maximum number of cards (5)." << endl;
        playerDone = true;
    }
}

void Blackjack::addDealerCard()
{
    if (dhandSize <= 5)
    {
        dhand[dhandSize] = 1 + (rand() % 11);
        dhandSize++;
    }
    else
    {
        dealerDone = true;
    }
}

void Blackjack::printHand()
{
    cout << "Your current hand is...\n";

    for (int i = 0; i < phandSize; i++)
    {
        cout << "    -" << phand[i] << "-    \n\n";
    }

    cout << "Dealer's current hand is...\n";

    for (int j = 0; j < dhandSize; j++)
    {
        cout << "    -" << dhand[j] << "-    \n\n";
    }
}

void Blackjack::sumHands()
{
    dhandSum = 0;
    phandSum = 0;
    for (int i = 0; i < dhandSize; i++)
    {
        dhandSum += dhand[i];
    }

    for (int j = 0; j < phandSize; j++)
    {
        phandSum += phand[j];
    }

    cout << "Current player hand sum is: " << phandSum << endl;
}

main.cpp

#include "stdafx.h"
#include "Blackjack.h"


int main()
{

    int exitGame = 1;

    do
    {
        Blackjack casino_royale;
        casino_royale.playGame();
        cout << "\nWould you like to play again? (1 - Yes, 2 - No)\n";
        cin >> exitGame;
    }while (exitGame == 1);


    cout << "\nThanks for playing!\n";
    system("pause");
    return 0;
}

Blackjack.h

#pragma once
#include "stdafx.h"


class Blackjack
{
public:
    Blackjack();
    void playGame();

private:
    int dhand[5];
    int phand[5];
    int dhandSize;
    int phandSize;
    int dhandSum;
    int phandSum;
    int phit;
    int pstand;
    bool playerDone;
    bool dealerDone;
    void addPlayerCard();
    void addDealerCard();
    void printHand();
    void sumHands();
};
\$\endgroup\$
0

5 Answers 5

15
\$\begingroup\$

Members Vs Globals

It's odd that you have global variables defined in your Blackjack.cpp file. These:

int dhand[5];
int phand[5];
int dhandSize;
int phandSize;
int dhandSum;
int phandSum;
int phit;
int pstand;
bool playerDone;
bool dealerDone;

Look like they should have been declared as class members (which now that you have added the header I can see that they are in there as well), rather than globals. The globals should be removed, they're just going to cause confusion.

What's in a deck

When you're dealing cards, you're deciding what card to add using a random generator.

phand[phandSize] = 1 + (rand() % 11);

This is ok as a start, however it's possible that the player could end up with 5 aces etc. Is that really what you want? With a pack of cards, there are many cards that have a value of ten (10,Jack,Queen,King) yet your current random approach thinks all card values are as likely. In reality, the chances of you getting each card decrease as cards of that value are removed from the deck. Consider adding a deck class that you initialise with 1 or more packs of shuffled cards when constructed then remove from the deck as each card is drawn. This would make your draws more realistic and allow you to reuse the deck class in any future card games you may construct (such as poker).

Duplicated Code - Player Vs Dealer

Dealers and players are almost the same. You're performing the same calculations in AddPlayerCard that you are performing in AddDealerCard. This could be generalised if you were for example to add the concept of a Player (dealers are players to, they're just automated) to your class. You could do something as simple as this:

struct Player {
    int dhand[5];
    int dhandSize;
    bool done;
};

Then you have a addCardFunction that looked more like this:

void Blackjack::addDealerCard(Player &player)
{
    if (player.dhandSize <= 5)
    {
        player.dhand[dhandSize] = 1 + (rand() % 11);
        player.dhandSize++;
    }
    else
    {
        player.done = true;
    }
}

The next step being to look at the Player abstraction to see if some of the functionality could be pushed from your BlackJack class into it. For example it could have a method to CalculateScore, based on the cards it is holding.

Adding this type of generalisation would also make it easier for you to extend your program so that it could for example support multiple players against the dealer.

Dealer Bias

After you deal your initial cards, you check if the dealer or player have Black Jack and declare them the winner if they do. If both players have 21, then the dealer is declared the winner. This is a good example of when extracting the functionality into a shared method would have helped you out. The checks you need to do are the same checks that are performed at the end of every round, except the end of round check supports a draw. If you extract the functionality into another method can call it from both places the game will become that little bit fairer for the players.

Constants are your friends

There are several places in your code that could benefit from the removal of 'magic' numbers. Replacing them with constants could help the readability of your code and reduce the chance of bugs. You might want to have a constant for 21 (possibly BlackJack), particularly since you have so many occurrences of it.

A constant for MaxScoreForDealerToHit of 17 would help to make your code self-documenting, the <17 becomes much more readable as:

if (dhandSum < MaxScoreForDealerToHit && dealerDone != true)

However, the main constants I would introduce are for Yes(1) and No(2). You ask the user some yes no questions and have the same values for the answer, so using a constant would really aid translation. Consider this (from your main loop):

int exitGame = Yes;

do
{
    Blackjack casino_royale;
    casino_royale.playGame();
    cout << "\nWould you like to play again? (1 - Yes, 2 - No)\n";
    cin >> exitGame;
}while (exitGame == Yes);

It becomes much more obvious that your exitGame variable should be called playAgain.

Letters for variable names don't cost the Earth

In context, you can figure out what dhand, dhandSize, dhandSum mean. However, they would be much more expressive if you pad them out with some missing letters: DealerHand, NumberOfCardsDealerHas, DealersCardTotal.

\$\endgroup\$
3
  • \$\begingroup\$ One small thing I noticed you didn't bring up: dealerDone != true is less intuitive than dealerDone == false or ! dealerDone \$\endgroup\$ Jul 1, 2016 at 0:13
  • \$\begingroup\$ @forsvarir Thank you very much for your reply! I've been slowly making the modifications that you suggested. Spent most of yesterday creating/testing a a deck class that I made. All of your suggestions are right on the money. \$\endgroup\$ Jul 1, 2016 at 19:36
  • \$\begingroup\$ @KyleMcCormick Thank you for pointing that out! I'll definitely change that. \$\endgroup\$ Jul 1, 2016 at 19:37
5
\$\begingroup\$

Blackjack is a pretty hard one to code in my experience. I couldn't finish it in the two weeks we had gotten in class (this was in Java)

The first thing I noticed was the use of srand so I'll focus on that since the first answer seems to cover a lot.

srand is considered harmful, this is an interesting talk detailing why. From this reference, the <random> header can be used to generate random numbers. Here's some example code, simulating dice, using that header (taken from the resource):

std::default_random_engine generator;
std::uniform_int_distribution<int> distribution(1,6);
int dice_roll = distribution(generator);

You can get clever with enum's for the Suit and Rank of theCard class, here's the header for Card:

#ifndef _CARD_H_
#define _CARD_H_
#include <string>

class Card
{
public:
    enum Rank
    {
        ACE = 1,
        TWO = 2,
        THREE = 3,
        FOUR = 4,
        FIVE = 5,
        SIX = 6,
        SEVEN = 7,
        EIGHT = 8,
        NINE = 9,
        TEN = 10,
        JACK = 11,
        QUEEN = 12,
        KING = 13
    };
    enum Suite
    {
        SPADE = 0,
        HEART,
        CLUB,
        DIAMOND
    };

    Card();

    void setId(const int& anId);

    Rank getRank() const;
    Suite getSuite() const;

    std::string toString() const;
    char getSuiteChar() const;

private:
    int id;
};
#endif

There is a little fun enum magic in the implementation regarding the rank and suit. Hint: it's simple math and two operators. Think about the structure of a deck of cards.

Here's the Deck class header we used in conjunction:

#ifndef _DECK_H_
#define _DECK_H_

#include "Card.h"

class Deck
{
public:
    Deck();

    void shuffle();
    const Card& getTopCard();

private:
    Deck(Deck&);

    Card cardSet[52];
    int topCardId;
};

#endif

Have fun!

\$\endgroup\$
3
  • \$\begingroup\$ @J Hache Thank you very much! Especially for going back and finding your past assignment to use for an example. The usage of the enumeration and the implementation of a card/deck class were great ideas, I used both of them last night. My code ended up looking similar to yours in the end, still working on it though. \$\endgroup\$ Jul 1, 2016 at 19:39
  • \$\begingroup\$ For the inclusion guard macros, do not use identified starting with an underscore. These names are reserved for the implementation of the runtime environment, which is the part you don't write. \$\endgroup\$ May 12, 2018 at 17:36
  • \$\begingroup\$ The identifiers _CARD_H_ and _DECK_H_ is a name reserved for the standard library. It’s also too simple and might clash in a large program. IOW, imitating what you see in system headers is exactly what you should not do. \$\endgroup\$
    – JDługosz
    May 13, 2018 at 0:12
3
\$\begingroup\$

Another handy class to include would be a card class. This way you can store the pip value as well as displayable characters for the card value and suit.

Also nesting these other classes inside the collection class(BlackJack) gives you more control over how they are used.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I used this idea after the previous answer suggested it. \$\endgroup\$ Jul 1, 2016 at 19:40
1
\$\begingroup\$

I stopped working on the game during the last two years of my computer science degree program and now in my free time I want to improve it. I'm looking for suggestions on how I can improve the game. Not just adding new features, but how I can improve the organization, eliminate unnecessary code, and stick to common (industry standard) programming practice.

Work it over with new language features, libraries, and style in mind. (I’m doing something similar.)

We have C++17 now, and your code was written for C++11.

I’m sure your skill has grown since then too, so the code shown above is obsolete. You might want to use that problem again, digging into structured binding, constexpr, and other new goodies. Please post your latest code in a new Question if you do so!

\$\endgroup\$
0
\$\begingroup\$

When dealing cards, you should have all the cards stored in an array with their status if they are dealt already or not yet (still part of the deck).

To deal a card:

a) a random number is generated up to the size of the deck and card still in the deck is counted sequentially. All cards already dealt is skipped.

b) the card is assigned to the player's hand and card status is changed to "dealt" and deck size is decreased by 1.

example:

    deck_size = 20;
    card_pos = random(deck_size); // generates random number from 0 - 19
    for (int i =0; i < card_pos; i++)
    {
       if (deck[i].isInDeck())
       {
           i++;
       }
    }

    chosen_card = deck[i];

    player_hand.new_card(chosen_card);

    deck[i].setStatus(DEALT); //isInDeck() will now return false

    deck.decrementSize(); // decreased by 1;
\$\endgroup\$
1
  • \$\begingroup\$ It's probably better to just deal from the front of a shuffled deck, as forsvarir's answer recommends (in "What's in a deck"). \$\endgroup\$ Mar 13 at 7:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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