Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This is a small program I wrote for GCSE computing, it operates using Vectors and that's pretty much it, wondering if you could tell me if there is anything I could do better.

BlackJackv2.cpp

#include "BlackJack v2.h"

int main() {
    // Create Vector with the players, using vec because of an undiefined amount   of players
    std::vector<Players> VecPlayers;

    // StrLine for entering players  
    std::string StrLine = "";
    // Used as an index for VecPlayers
    unsigned int CountPlayer = 0;

    // Give players that are playing then add them to the vector
    std::cout << "Please give the players.\n: ";
    while (std::cin >> StrLine) {
        if (StrLine == ".") break;
        VecPlayers.push_back(Players());
        VecPlayers[CountPlayer].Name = StrLine;
        CountPlayer++;
        std::cout << ": ";
    }

    // Clear the input buffer
    std::cin.ignore('/n');
    std::cin.clear();

    // Reset CountPlay so that it starts at the first index value
    CountPlayer = 0;
    // Answer used for user input
    char Answer = 'N';

    do {
        // Seed for RNG
        srand(int(time(NULL)));
        // Start the turn of the player and say his name
        std::cout << std::string(100, '\n') << VecPlayers[CountPlayer].Name << "'s Turn: ";
        // Press enter to continue
        std::cin.ignore();

        // Give the player two cards
        VecPlayers[CountPlayer] = AddCard(VecPlayers[CountPlayer], 2);

        // Show the player the cards
        DisplayCard(VecPlayers[CountPlayer]);

        // Add the cards to together and show them
        VecPlayers[CountPlayer] = SumCard(VecPlayers[CountPlayer]);

        // Ask if they want another card and ask for input
        std::cout << std::endl << "Do you want another card? Y/N? \n:";
        std::cin >> Answer;

        // If they want another card
        while (Answer == 'Y') {
            // Give the player one card
            VecPlayers[CountPlayer] = AddCard(VecPlayers[CountPlayer], 1);

            // Show all the playerscards
            DisplayCard(VecPlayers[CountPlayer]);

            // Add the cards toghether
            VecPlayers[CountPlayer] = SumCard(VecPlayers[CountPlayer]);
            // If your cards sum is less than or equal to 21
            if (VecPlayers[CountPlayer].Sum <= 21) {
                // Ask if the want another card
                std::cout << std::endl << "Do you want another card? Y/N? \n:";
                std::cin >> Answer;
                // Capitalise all input so no need for or operator
                Answer = toupper(Answer);
            }
            // If they have too many cards
            else {
                std::cout << "Bust! Bad luck, the RNG gods were not on your side!" << std::endl;
                // Exit the while loop
                break;
            }
        }

        // If the current player is the last player in the vector
        if ((VecPlayers.size() - 1) == CountPlayer) {
            // GreatestSum is used to see who has the best set
            // WinnersIndex stores the winners index
            int GreatestSum = 0, WinnersIndex = 0;

            // Go through each index to see who has the largest sum
            for (unsigned int Count = 0; Count < VecPlayers.size(); Count++) {
                // Added so that new rounds have no cards
                VecPlayers[Count].Cards = 0;

                // If the current sum from the vector is the largest found then that is the winner
                if (VecPlayers[Count].Sum > GreatestSum && VecPlayers[Count].Sum < 22) {
                    // Assign the Sum of the current player to GreatestSum
                    GreatestSum = VecPlayers[Count].Sum;
                    // Assign the Index of the current player to WinnersIndex
                    WinnersIndex = Count;
                }
            }

            // Print out the winners name and sum of his cards
            std::cout << std::string(100, '\n') << "The winner is " << VecPlayers[WinnersIndex].Name
                << " with the sum of " << VecPlayers[WinnersIndex].Sum << "\n:";
            // Press enter to continue
            std::cin.ignore();
            CountPlayer = 0;



        }
        else CountPlayer++;
        std::cin.ignore();
    } while (true);
    return 0;
}

BlackJack v2.h

#include <iostream>
#include <string>
#include <vector>
#include <time.h>

struct Players {
    std::string Name = "";
    unsigned int Wins = 0;
    unsigned int Cards = 0;
    int Sum = 0;
    unsigned int IndexSuit[10] = { 0 }; // Suit is from 0 to 3
    unsigned int IndexCard[10] = { 0 }; // Card is from 0 to 12
};

int RandomNumber(int Start, int End) {
    int RNG = (rand() % End) + Start;
    return RNG;
}

Players AddCard(Players Player, unsigned int NumberCards) {
    for (unsigned int Count = 0; Count < NumberCards; Count++) {
        Player.IndexSuit[Player.Cards] = RandomNumber(0, 3);
        Player.IndexCard[Player.Cards] = RandomNumber(0, 12);
        Player.Cards++;
    }
    return Player;
}

void DisplayCard(Players Player) {
    // Displays Suit of card 
    const std::string CardSuit[] = { "Hearts", "Diamonds", "Clubs", "Spades" };
    // Displays Name of card
    const std::string CardName[] = { "Ace", "Two", "Three", "Four", "Five", "Six", 
        "Seven", "Eight", "Nine", "Ten", "Jack", "Queen", "King" };

    std::cout << "You have:" << std::endl << std::endl;
    for (unsigned int CardCount = 0; CardCount < Player.Cards; CardCount++) {
        std::cout << "The " << CardName[Player.IndexCard[CardCount]] << " of " 
            << CardSuit[Player.IndexSuit[CardCount]] << std::endl;
    }

    return;
}

Players SumCard(Players Player) {
    Player.Sum = 0;


    for (unsigned int CardCount = 0; CardCount < Player.Cards; CardCount++) {
        if (Player.IndexCard[CardCount] < 10) {
            std::cout << (Player.IndexCard[CardCount] + 1);
            Player.Sum = Player.Sum + (Player.IndexCard[CardCount] + 1);
        }
        else if (Player.IndexCard[CardCount] == 0) {
            std::cout << "You have an Ace, (H)igh or (L)ow? \n:" << std::endl;
            char Answer = ' ';
            std::cin >> Answer;
            if (Answer == 'H') {
                std::cout << "11";
                Player.Sum = 11;
            }
            else {
                std::cout << "1";
                Player.Sum = 1;
            }
        }
        else {
            std::cout << "10";
            Player.Sum = Player.Sum + 10;
        }
        if ((CardCount + 1) == Player.Cards) std::cout << " = ";
        else std::cout << " + ";
    }
    std::cout << Player.Sum << std::endl;
    return Player;
}
share|improve this question
1  
Way to many usless comments. Comments are to explain WHY the code explains what is happening (by the use of good function/variable names). – Loki Astari Dec 8 '15 at 6:56
up vote 4 down vote accepted

You should write self documenting code.

This basically means breaking your code up into logical well named units. Units are functions and classes. Classes should represents objects in your system.

I would have layed out main something like this:

int main()
{
     std::vector<Player>    players = getPlayers();
     Shoe                   cards(6); // initialize with 6 decks.

     for(;;) {

        Game    game(players, cards);

        game.dealCards();
        for(player: players) {
            game.getExtraCards(player);
        }
        game.getExtraCards(); // for the dealer.

        game.payWinners();
     }
}

Couple of classes I would build:

 Game:    A class to hold state about the current game.
 Player:  A class to hold state about players
 Cards:   A class to hold the different types of cards.
          Mainly so they are easy to print.
 Shoe:    A class to hold all the cards from several decks of cards.
          So you can deal from the deck. Then shuffle when you have
          used all the cards.

Seeding Random Number

srand(int(time(NULL)));

You should seed the random number generator only once during an application run. By seeding it multiple times you ruin the distribution. So move this out of the loop and put it just after main starts.

Random Number generation.

int RandomNumber(int Start, int End) {
    int RNG = (rand() % End) + Start;
    return RNG;
}

This is probably the anti pattern for getting a rand number (and it has a bug). This is because the space is for random numbers is not even so the chance for any particular number is not the same.

The rand() function generates a number in the range 0-RANDMAX. For arguments sake lets say RANDMAX 32768. Your want a number in the range 1-6.

This means.

 1:   Probability:  5462/32768   // Notice this has a slightly higher probability
 2:   Probability:  5462/32768   // Notice this has a slightly higher probability
 3:   Probability:  5461/32768
 4:   Probability:  5461/32768
 5:   Probability:  5461/32768
 6:   Probability:  5461/32768

The Bug:

 Start = 8;
 End   = 10;
 int RNG = (rand() % End) + Start;  // Number in the range.  8 -> 17

The better way to write this is:

int RandomNumber(int start, int end) {
    int rng = end - start + 1;
    int max = RANDMAX / rng * rng; // Integer arithmetic so will be
                                   // less than RANDMAX if it does
                                   // not divide exactly.
    int rnd;
    // Get a value from [0, max)
    // Discards numbers outside the range.
    // So that each number has an equal probability.
    do {
        rnd = rand();
    }
    while(rnd >= max);

    // Generate the result.
    return (rnd % rng) + start;
}

Also rand() is old and is known to be pretty bad. You should be using the new random number generator that was provided in C++11 to solve this specific problem.

http://en.cppreference.com/w/cpp/numeric/random

Generating a pack of cards

Players AddCard(Players Player, unsigned int NumberCards) {
    for (unsigned int Count = 0; Count < NumberCards; Count++) {
        Player.IndexSuit[Player.Cards] = RandomNumber(0, 3);
        Player.IndexCard[Player.Cards] = RandomNumber(0, 12);
        Player.Cards++;
    }
    return Player;
}

The problem here is that you are likely to get a couple of cards that are the same (thus stacking your deck slightly).

What you should do is generate all the cards you expect to find in the deck then shuffle the deck to get a random order.

class Shoe
{
    std::vector<Card>                  cards;
    std::vector<Card>::iterator        nextCard;   
    std::random_device                 rd;
    std::mt19937                       randomGenerator;
    public:
        Shoe(int numberOfDecks)
            : randomGenerator(rd)
        {
            // Create all the cards.
            for(int deck = 0;deck < numberOfDecks; ++deck) {
                for(int card=0; card < 52; ++card) {
                    cards.push_back(Card(card));
                }
            }
            shuffle();
       }
       void shuffle()
       {
            std::shuffle(std::begin(cards), std::end(cards), randomGenerator);

            nextCard = std::begin(cards);
       }
       Card  deal()
       {
           if (nextCard == std::end(cards)) {
               shuffle();
           }
           return *nextCard++
       }
}
share|improve this answer
    
Hey I was just thinking, would a more elegant solution to the deck stacking be that every time you pick a card check through the vector to see if it has been used before, of course you couldn't have more than one deck. Perhaps you could just create a boolean function that checks if there has been a card played that is equal to the one that you have just randomly created and if so pick a new card? I'm systematically going through all my code trying to make it more object oriented. Your help has made me look at my own code and see that there is so much more that I could learn. – Thomas Broadbent Dec 9 '15 at 22:55
1  
There are only 52 cards. Why not create an object that holds 52 cards. Then shuffle and deal them. The memory footprint would be minimal. – Loki Astari Dec 9 '15 at 22:57

Use Functions

Your main() function is quite long. I suggest breaking it out into separate functions. You've done a little of that with the functions in your header (why are you putting entire functions in your headers?), but it needs more. For example, I'd gather the players' names in one function and run the main game loop in another function. I'd further break the game loop into a couple of functions that prompt each player in turn and then checks to see if anyone has won yet.

Use enumerated types

In your Players struct, I see this:

unsigned int IndexSuit[10] = { 0 }; // Suit is from 0 to 3

Suits should be an enumerated type. Something like:

enum {
    Suit_Hearts = 0,
    Suit_Diamonds,
    Suit_Clubs,
    Suit_Spades
} Suit;

Then you can create the array like this:

Suit IndexSuit[10] = { Suit_Hearts };

Use Classes

You've made Players a struct with no methods. It would probably be better if it were a class or at least had some methods. The AddCard() function looks like a good candidate for something that should be a method on Players. Possibly DisplayCard(), too. And definitely SumCard().

share|improve this answer
    
Thanks for the help, I didn't realise I was only suppose to declare functions in headers. I'll know for next time – Thomas Broadbent Dec 8 '15 at 10:31
1  
@ThomasBroadbent: Defining functions in headers is fine. Though if you do so, they should be really short (so they are likely to be inlined and inlining is worth it), and have to be marked inline. – Deduplicator Dec 8 '15 at 13:17

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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