A Simple Console Based Keno/Lottery Game in C++

Question

Earlier this week I provided this answer to the Original Poster of the question. Since I've already shown that I am not a C++ Guru, I have made the changes I suggested and I'm posting them for review. My question is what alternative Standard Template Library constructs I could have used to implement this. In case you aren't aware, copying is a major form of flattery.

The Basics of the game are:

• The user creates a balance of credits to play the game.

• Per game round the user enters a bet.

• The user then has the choice of either picking between 5 and 10 numbers between 1 and 60 or allowing the computer to pick between 5 and 10 numbers between 1 and 60.

• Each game round is scored based on the number of picks and the number of matches of those picks. Winnings are applied to the balance, bets reduce the balance.

• A zero balance prevents game play and allows the user to add credits.

The goals I had when writing this alternate solution are:

• Break the one file solution into multiple header and source files to provide better object oriented programming and reuse.

• Replace all C programming language constructs with C++ programming.

• Improve the readability and maintainability of the code.

• Follow the Single Responsibility Principle.

• Follow the DRY programming Principle.

• Follow the KISS principle.

• Reduce the number of lines of code in the solution.

Note: I was not successful in reducing the total number of lines of code, at least partially because breaking the code into header and source files adds some overhead lines of code, and because adding functions adds at least 3 lines of code for every new function. The original solutions is approximately 375 lines of code this alternate solution is approximately 440 lines of code.

One of the few major changes to the code, one is that the function kenoGameplay(Bank &credits) was turned into a class. The other is that an std::map() of multiple maps was used to replace a rather large case statement with embedded if statements in Award::awardWon(Bank &bank). A third was to decouple the Award class from the Bank class by adding another interface to the Bank class. A fourth was to change a C style array of integers into a std::vector. The final major change is that the random number generator now returns vectors of unique random numbers rather than a single random number and the interface is no longer a static member.

Possible Bugs in the Original that are Repeated Here

• If this were truly a Keno Game rather than a Lottery Game, then the bet should be used as a multiplier of the award, there is currently no reason to bet anything other than 1.
• Rather than forcing the user to bet every time allow them to choose to keep the same bet or change it.
• This program still uses the C standard library rand() function rather than more modern random number generators. This is offset somewhat by the fact that the implementation is now hidden and changing this will only force relinking after RandomNumber.cpp is recompiled.

Because I used Visual Studio 2015, this alternate solution is not portable without edits to remove stdafx.h from source files and replace #pragma once with guard bandes in the header files.

A specific question about the code is there a single STL construct I can use to replace this function, I don't think std::equal() will work since one vector is always length 15 and the other will vary between length 5 and length 10.

void KenoWinCalculator::calculateHits()
{                                  
    for each (auto playerPick in m_playerPick)
    {
        if (std::find(m_ComputerPick.begin(), m_ComputerPick.end(), playerPick) != m_ComputerPick.end())
        {
            m_hits++;
        }
    }
}

A second specific question I have is would it be better to use std::count() over std::find() in the above function.

EDIT
While I am specifically interested in STL improvements, all observations are good, since I'm far from perfect.

GetUserInput.h

#pragma once
#include <iostream>
#include <string>

template<typename T>
T PromptUserReturnResponse(const char* prompt)
{
    T response;

    std::cout << prompt << std::endl;
    std::cin >> response;
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

    return response;
}

kenoGameConstants.h

#pragma once
static const int MIN_PICK_VALUE = 1;
static const int MAX_PICK_VALUE = 60;
static const int COMPUTER_PICK_COUNT = 15;

KenoGame1.cpp

#include "stdafx.h"
#include <iostream>
#include <string>
#include "Bank.h"
#include "KenoGamePlay.h"
#include "GetUserInput.h"

std::string welcomeUser()
{
    std::cout << "******** WELCOME TO WORLD OF KENO ************** \n \n";
    std::string name = PromptUserReturnResponse<decltype(name)>("Please enter your name \n");
    std::cout << "Hello! " << name << "...\n\n" << std::endl;

    return name;
}

Bank StartTheBank()
{
    Bank credits;

    credits.setBankBalance(PromptUserReturnResponse<int>("Please insert money"));

    return credits;
}

int main() {
    int programStatus = EXIT_SUCCESS;

    std::string name = welcomeUser();
    kenoGamePlay kenoGameController(StartTheBank());
    programStatus = kenoGameController.GameLoop();

    return programStatus;
}

kenogameplay.h

#pragma once
#include <iostream>
#include <vector>
#include "Bank.h"
//#include "RandomNumber.h"

class kenoGamePlay
{
private:
    Bank &m_credits;
    class RandomNumber *m_NumberGenerator;

protected:
    int PlayOneGame();
    std::vector<int> QuickPickNumbers();
    std::vector<int> PlayerPicksNumbers();
    bool IsQuickPick();
    void ValidateNumberAndAddOrReport(int num, std::vector<int> &gameNumbersToPlay);
    void DisplayIntegersInVector(std::vector<int> numbersToPrint) const
    {
        for each (auto num in numbersToPrint)
        {
            std::cout << num << "  ";
        }
        std::cout << "\n";

    }

public:
    kenoGamePlay(Bank &credits);
    ~kenoGamePlay() = default;
    int GameLoop();

};

kenogameplay.cpp

#include "stdafx.h"
#include <iostream>
#include <vector>
#include <string>
#include <algorithm>
#include "kenoGameConstants.h"
#include "kenogameplay.h"
#include "KenoWinCalculator.h"
#include "Award.h"
#include "GetUserInput.h"
#include "RandomNumber.h"

static const size_t QUIT_VALUE = 100;
static const size_t MAX_NOS_PICKED = 10;
static const size_t MIN_NOS_PICKED = 5;
static const size_t INSERT_MONEY = 1;

kenoGamePlay::kenoGamePlay(Bank &credits)
    : m_credits{credits}
{
    m_NumberGenerator = new RandomNumber();
}

std::vector<int> kenoGamePlay::QuickPickNumbers()
{
    int quickPickCount = 0;
    char* Prompt = "How many number do you want the computer to pick (5-10)?";  
    do {
        quickPickCount = PromptUserReturnResponse<decltype(quickPickCount)>(Prompt);
        Prompt = "Invalid count value. Choose between 5 and 10 only.?";
    } while (quickPickCount < MIN_NOS_PICKED || quickPickCount >  MAX_NOS_PICKED);

    return m_NumberGenerator->GenerateAndGetVectorOfUniqueRandomNumbers(quickPickCount);
}

void kenoGamePlay::ValidateNumberAndAddOrReport(int num, std::vector<int> &gameNumbersToPlay)
{
    if (num >= MIN_PICK_VALUE && num <= MAX_PICK_VALUE) {
        if (std::count(gameNumbersToPlay.begin(), gameNumbersToPlay.end(), num) != 0)
        {
            std::cout << "You already picked " << num << ". Please select another number. \n";
        }
        else
        {
            gameNumbersToPlay.push_back(num);
        }
    }
    else
    {
        std::cout << "You entered an invalid number. Please try again \n";
    }
}

std::vector<int> kenoGamePlay::PlayerPicksNumbers()
{
    std::cout << "Pick between " << MIN_NOS_PICKED << " and " << MAX_NOS_PICKED <<
        " unique numbers between " << MIN_PICK_VALUE << " and " << MAX_PICK_VALUE << 
        ". Enter " << QUIT_VALUE << " to QUIT entering numbers\n";

    int num = 0;
    std::vector<int> gameNumbersToPlay;
    while (gameNumbersToPlay.size() < MAX_NOS_PICKED)
    {
        num = PromptUserReturnResponse<decltype(num)>("Enter a number>>");
        if (num == QUIT_VALUE)
        {
            if (gameNumbersToPlay.size() >= MIN_NOS_PICKED)
            {
                return gameNumbersToPlay;
            }
            else
            {
                std::cout << "You haven't selected enough numbers yet, please pick more numbers.\n";
            }
        }
        else {
            ValidateNumberAndAddOrReport(num, gameNumbersToPlay);
        }
    }

    return gameNumbersToPlay;
}

int kenoGamePlay::GameLoop()
{
    int programStatus = EXIT_SUCCESS;
    char continueGameplay = 'y';
    bool isBetInvalid = true;
    int insertMoneyReduceBet = 0;

    while (programStatus == EXIT_SUCCESS && continueGameplay != 'n')
    {
        if (m_credits.isEnoughBalanceToPlayGame() || isBetInvalid == true)
        {   // check if enough balance to play game
            m_credits.setBet(PromptUserReturnResponse<int>("Place your bet"));

            if (m_credits.isBetValid()) {
                m_credits.updatebankBalance();
                programStatus = PlayOneGame();
                continueGameplay = PromptUserReturnResponse<decltype(continueGameplay)>("GAME OVER \nDo you want to play another round \n'y' to continue and 'n' to quit playing \n");
            }
            else
            {
                std::cout << "Invalid bet value." << std::endl;
                isBetInvalid = true;
            }
        }
        else
        {
            std::cout << "Not enough balance/valid bet. Please insert money( press 1) or place valid bet(press 2) to max bank balance " << m_credits.getBankBalance() << std::endl;
            std::cin >> insertMoneyReduceBet;

            if (insertMoneyReduceBet == INSERT_MONEY) {
                insertMoneyReduceBet = PromptUserReturnResponse<decltype(insertMoneyReduceBet)>("Insert money ");
                m_credits.setBankBalance(insertMoneyReduceBet);
            }
            else
                isBetInvalid = true;
        }
    }

    return programStatus;
}

int kenoGamePlay::PlayOneGame()
 {
    int programStatus = EXIT_SUCCESS;
    std::vector<int> nosPicked = (IsQuickPick()) ? QuickPickNumbers() : PlayerPicksNumbers();

    KenoWinCalculator winCalc(nosPicked);
    winCalc.generateComputerPick(m_NumberGenerator);
    winCalc.calculateHits();

    Award wonAward(winCalc);
    wonAward.awardWon(m_credits);

    std::vector<int> playerPickedValues = winCalc.getPlayerPickValues();

    std::cout << "Your selection: \t";
    DisplayIntegersInVector(playerPickedValues);

    std::cout << "\nComputer selection: \t";
    DisplayIntegersInVector(winCalc.getComputerPick());
    std::cout << "\nno of hits: " << winCalc.getHitValue() << "\n" << "Your bank balance is: $" << m_credits.getBankBalance() << "\n";
    std::cout.flush();

    return programStatus;
}

bool kenoGamePlay::IsQuickPick()
{
    return (PromptUserReturnResponse<char>("Do you want the computer to pick the numbers for you? [y]es or [n]o") == 'y');
}

Bank.h

#pragma once
class Bank {
private:
    int m_bet;
    int m_bankBalance;

public:
    Bank() : m_bet{ 0 }, m_bankBalance{ 0 } {}  // this class maintains bet and player credit meter
    ~Bank() = default;
    void setBet(int betValue) { m_bet = betValue; }
    void setBankBalance(int balance) { m_bankBalance += balance; }
    void updatebankBalance() { m_bankBalance -= m_bet; }
    void depositWinnings(int winnings) { m_bankBalance += winnings; }
    int getBankBalance() const { return m_bankBalance; }
    bool isEnoughBalanceToPlayGame() const { return m_bankBalance >= m_bet; }
    bool isBetValid() const { return m_bet <= m_bankBalance && m_bet > 0; }
};

RandomNumber.h

#pragma once
#include <vector>
class RandomNumber {                                         // Random number generator
private:
    std::vector<int> UniqueRandomNumbersContainer;

protected:
    int randomNumberGenerator();

public:
    RandomNumber();
    ~RandomNumber();
    void resetUniqueRandomNumbersContainer()
    {
        if (!UniqueRandomNumbersContainer.empty())
        {
            UniqueRandomNumbersContainer.clear();
        }
    }
    std::vector<int> getaVectorOfUniqueRandomNumbers() const { return UniqueRandomNumbersContainer; }
    std::vector<int> GenerateAndGetVectorOfUniqueRandomNumbers(size_t UniqueRandomNumberCount)
    {
        generateUniqueRandomNumbers(UniqueRandomNumberCount);
        return UniqueRandomNumbersContainer;
    }
    void generateUniqueRandomNumbers(size_t UniqueRandomNumberCount);

};

RandomNumber.cpp

#include <ctime>
#include <algorithm>
#include "stdafx.h"
#include "RandomNumber.h"
#include "kenoGameConstants.h"

RandomNumber::RandomNumber()
{
    srand(static_cast<unsigned int>(time(0)));
}

RandomNumber::~RandomNumber()
{
    resetUniqueRandomNumbersContainer();
}

int RandomNumber::randomNumberGenerator() {
    int randomRange = MIN_PICK_VALUE + MAX_PICK_VALUE;
    return (rand() % randomRange);
}

void RandomNumber::generateUniqueRandomNumbers(size_t UniqueRandomNumberCount)
{
    if (!UniqueRandomNumbersContainer.empty())
    {
        UniqueRandomNumbersContainer.clear();
    }
    size_t generatedCount = 0;
    while (generatedCount < UniqueRandomNumberCount)
    {
        int rngValue = randomNumberGenerator();
        if (std::find(UniqueRandomNumbersContainer.begin(), UniqueRandomNumbersContainer.end(), rngValue) == UniqueRandomNumbersContainer.end())
        {
            UniqueRandomNumbersContainer.push_back(rngValue);
            generatedCount++;
        }
    }
}

award.h

#pragma once
#include "KenoWinCalculator.h"
#include "Bank.h"
#include <map>

class Award {
private:                                                                       // This class maintains pay table

    typedef std::map<int, int> HitMap;
    typedef std::map<int, HitMap> PlayerPickMap;

    int m_picks;
    int m_hits;
    PlayerPickMap MapSelector;

public:
    Award(const KenoWinCalculator &calc);
    ~Award();
    void awardWon(Bank &bank);
};

Award.cpp

#include <iostream>
#include "award.h"
#include "KenoWinCalculator.h"

Award::Award(const KenoWinCalculator &calc)
{
    m_picks = calc.getPlayerPickCount();
    m_hits = calc.getHitValue();

    // In each of the follow tuples the first number is the number of hits or matches
    // and the second number is the dollar reward. ie {4, 50} means 4 hits and $50.
    HitMap Pick5Hits = { {4, 50}, { 5, 100} };
    HitMap Pick6Hits = { {4, 100}, {5, 120}, {6, 150} };
    HitMap Pick7Hits = { {3, 80}, {4, 90}, {5, 100}, {6, 120}, {7, 150} };
    HitMap Pick8Hits = { {3, 80}, {4, 90}, {5, 100}, {6, 120}, {7, 150}, {8, 175} };
    HitMap Pick9Hits = { {3, 80}, {4, 90}, {5, 100}, {6, 120}, {7, 150}, {8, 175}, {9, 200} };
    HitMap Pick10Hits = { {3, 80}, {4, 90}, {5, 100}, {6, 120}, {7, 150}, {8, 175}, {9, 200}, {10, 250} };

    MapSelector = { { 5, Pick5Hits}, {6, Pick6Hits}, {7, Pick7Hits}, {8, Pick8Hits}, {9, Pick9Hits}, {10, Pick10Hits} };
}

Award::~Award()
{
    for each(auto hitmap in MapSelector)
    {
        hitmap.second.clear();
    }
    MapSelector.clear();
}

void Award::awardWon(Bank &bank) {
    int amtWon = 0;

    auto correctHitMap = MapSelector.find(m_picks);
    if (correctHitMap != MapSelector.end())
    {
        HitMap HitMapTosearch = correctHitMap->second;
        auto HitValueMap = HitMapTosearch.find(m_hits);
        if (HitValueMap != HitMapTosearch.end())
        {
            amtWon = HitValueMap->second;
        }
    }

    if (amtWon > 0) {
        std::cout << "Congratulations you won $" << amtWon << "!\n";
        bank.depositWinnings(amtWon);
    }
    else
        std::cout << "Sorry you lost\n";

    std::cout << std::endl;     // flush the output
}

KenoWinCalculator.h

#pragma once
#include <vector>
#include "RandomNumber.h"

class KenoWinCalculator {

private:
    std::vector<int> m_playerPick;                              // vector is used because the size is not fixed,
    std::vector<int> m_ComputerPick;                            // player can choose any picks between 5 and 10 but computer's is fixed
    int m_hits = 0;

public:
    KenoWinCalculator(const std::vector<int> &playerPick);
    ~KenoWinCalculator() = default;
    void generateComputerPick(RandomNumber* NumberGenerator);   // Generate COMPUTER_PICK_COUNT unique computer picked random values
    void calculateHits();                                       // Calculates no of common nos between player and computer pick
    int getHitValue() const { return m_hits; }
    int getPlayerPickCount() const { return static_cast<int>(m_playerPick.size()); }
    const std::vector<int>& getPlayerPickValues() const { return m_playerPick; }
    std::vector<int> getComputerPick() const { return m_ComputerPick; }
};

KenoWinCalculator.cpp

#include <algorithm>
#include "kenoGameConstants.h"
#include "RandomNumber.h"
#include "KenoWinCalculator.h"


KenoWinCalculator::KenoWinCalculator(const std::vector<int> &playerPick) {
    m_playerPick = playerPick;
}

void KenoWinCalculator::generateComputerPick(RandomNumber* NumberGenerator) {                           // Generate 10 unique computer picked random values
    m_ComputerPick = NumberGenerator->GenerateAndGetVectorOfUniqueRandomNumbers(COMPUTER_PICK_COUNT);
}

void KenoWinCalculator::calculateHits()
{                                  
    for each (auto playerPick in m_playerPick)
    {
        if (std::find(m_ComputerPick.begin(), m_ComputerPick.end(), playerPick) != m_ComputerPick.end())
        {
            m_hits++;
        }
    }
}

Answer 1

A Serious Bug

I am sorry to be the bearer of bad message, but your whole programs behavior is currently undefined. You have stepped into the C++ reference-lifetime-trap. Lets take a look at this piece of code:

class kenoGamePlay
{
private:
    Bank &m_credits;
    //...
};

m_credits is a reference to a Bank-object. References as class members are rare and very dangerous, so let's pull out our alarm lights and go investigate whether the reference is justified and used correctly:

int main() {
    //...
    kenoGamePlay kenoGameController(StartTheBank());
    //...
}

Bingo. You're assigning a non-const, non-rvalue reference from a temporary, which is actually not allowed by the standard (and my compiler rejects it correctly). However, even if it were, you'd have a problem anyways: Since the lifetime of the temporary ends immediately, you now have a dangling reference that points to an invalid location in memory (= a location where no object of type Bank lives). Doing accesses through such a reference is undefined behavior, and thus your whole program becomes undefined.

Fixing this, however, is not at all difficult: Just remove the reference and let m_credits be a normal object of type Bank.

In general, avoid using references as members unless you are 100% percent sure of what you're doing. In almost all cases, you are running a high risk of shooting yourself in the foot and blowing your whole leg off in the process.

Regarding KenoWinCalculator::calculateHits()

As far as my knowledge of the STL goes, there is no single function that would do what you require here. However, I came up with an implementation using both std::count_if as well as std::find, which, in my opinion, is about as concise as it will get:

void KenoWinCalculator::calculateHits()
{                                  
    m_hits += std::count_if(m_playerPick.begin(), m_playerPick.end(), [this] (int a) {
        return std::find(m_ComputerPick.begin(), m_ComputerPick.end(), a) != m_ComputerPick.end();
    });
}

About STL Usage

1. if (std::count(gameNumbersToPlay.begin(), gameNumbersToPlay.end(), num) != 0): Why are you using std::count here? You only care if an element exists or not, so you can terminate your search as soon as you have found a matching element, which is exactly how std::find behaves.

2. Depending on how much you love the STL, you could consider replacing most of your for-loops with std::for_each. However, this is a disputable refactoring; many people are going to disagree with that style. Ultimately, it's a matter of personal taste.

Various Hints and Tips

1. for each (... in ...) is not valid C++. In fact, when first reading your code, I was really taken aback because I thought that this shouldn't ever compile. However, it turns out (after researching) that this is actually a MSVC extension, albeit a really useless and horrible one, in my opinion. Please, please, please use the standard for (... : ...) syntax, lest you want a mob of C++ programmers with pitchforks and and burning torches show up at your door.

2. Don't use std::endl. It's bad, because a) '\n' does the job just fine, and b) std::endl actually also calls flush() on your output stream, which is something you very rarely want. Just stick with '\n' and call flush() directly when you mean to.

3. Order your includes. Why? Because ordering includes makes it easier to verify whether all necessary includes are actually there, in two different ways: One, it makes it easier to manually verify that all necessary headers are there and two, the compiler will complain if you are missing an include in a header file if that header file is included before its missing header is included

   (For example, imagine A.h depends on including vector. However, the author of A.h forgot the include, and now you're using the header in your project file B.cpp, which also requires #include <vector>. If you first include A.h and then vector, your compiler will complain that it can't resolve std::vector in A.h. If you include vector first, however, this error does not surface because now the include is there. This is known as include shadowing.)

   So how should you be ordering those includes? First of all, nearly all people agree on alphabetic ordering. Secondly, it makes sense to group headers into categories such as "files from the current project", "library headers", "STL headers" etc. If you do this, it is recommended to write, from top to bottom, first the header the current .cpp file is implementing (if any), then headers from the current project (i.e. local/internal headers), then headers from different projects and libraries (i.e. external headers) and finally standard headers. This is most likely to prevent include shadowing bugs due to each header being checked in its implementation file (given that such a file exists).

4. Always use std::size_t over size_t; the existence of size_t in the global namespace is not guaranteed by the standard, but in the std namespace it is.

5. Avoid including iostream everywhere. The reason for this is that iostream usually injects static instances for cout, cerr and cin which can lead to binary bloat. More importantly, however, iostream is a code smell, because using std::cout and family is constraining your users and yourself. Imagine, for example, a bot, which wants to run some 1000 or 10000 games of Keno and log the results to a file, or a websocket, or any other I/O device. Currently, the only way for that bot to achieve this is by redirecting standard output, but that might not always be possible. Also, what if the bot actually only wants to log the output of DisplayIntegersInVector() to its destination, but doesn't care for the rest you write? Currently, it would need to parse the standard output for that, which, again, might be constraining or unfeasible.

   But if iostream is bad, how do you implement output functions? The answer is by refactoring your output functions to take an std::ostream as parameter and write to that instead. Syntax-wise, there is no change at all; the offered degree of flexibility, however, is much, much higher. (Don't forget to change #include <iostream> to #include <ostream>, though).

6. kenoGamePlay::DisplayIntegersInVector has two issues:

   1. Why is that function part of kenoGamePlay? You don't do anything related to the class at all; this function should be freestanding.
   2. The parameter numbersToPrint should not be passed by value, but by const& instead. std::vector is expensive to copy, and the function does nothing at all that would justify this extra cost.

7. Why are most member functions of kenoGamePlay protected? Is this class intended to be derived from? Doesn't look like it very much, since you have no virtual methods (not even a virtual destructor). If you still intend to derive from this, you should rethink your interface, if not, those members should be private.

8. Prefer using over typedef. It's more readable and more flexible.

9. Don't use single-letter string literals when not absolutely required. Writing "\n" hurts performance, especially when '\n' does the same thing in the same amount of characters.

10. You don't need to default all destructors manually; the compiler will generate a destructor for you automatically.

---

The bug I've found is somewhat unfortunate, but easy to fix. Apart from that, there are a lot of little things that you could do to improve your code, but apart from point 1 I didn't spot anything that would require immediate changes. However, it would be nice if you'd strive to make this code portable, i.e. remove #include "stdafx.h". Also, there are some irregularities in how you name things; you should decide on a coherent style for the sake of readability.

Regarding STL usage, I didn't actually find much that could be improved. Apart from replacing legacy C functionality (i.e. rand() and time()), there is not much to do in regards to making your code more C++-y.

Overall, I enjoyed reading your code. Keep up the good work!