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

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
4
down vote

favorite












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 << "...nn" << 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_creditscredits

m_NumberGenerator = new RandomNumber();


std::vector<int> kenoGamePlay::QuickPickNumbers()
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 numbersn";

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()

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 lostn";

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++;









share|improve this question





















  • Just to make sure, are you only interested in improvements regarding STL usage or would you also like general hints and corrections?
    – Ben Steffan
    Jan 5 at 20:08










  • @BenSteffan General hints and suggestions are good, but I am interested in the STL usage. I'm aware that I could improve my naming conventions, the use of Camel Case versus First Cap is inconsistent. Feel free to review everything since it is a learning experience no matter what.
    – pacmaninbw
    Jan 5 at 20:14






  • 1




    Fine, then I'll go ahead.
    – Ben Steffan
    Jan 5 at 20:15










  • Does your program actually compile? A conforming compiler should reject it.
    – Ben Steffan
    Jan 5 at 21:14






  • 1




    I think I remember that Visual Studio allowed that one to slip. Give me 5 minutes, I'm nearly done with my answer that includes a description of what I am talking about.
    – Ben Steffan
    Jan 5 at 21:32
















up vote
4
down vote

favorite












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 << "...nn" << 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_creditscredits

m_NumberGenerator = new RandomNumber();


std::vector<int> kenoGamePlay::QuickPickNumbers()
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 numbersn";

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()

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 lostn";

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++;









share|improve this question





















  • Just to make sure, are you only interested in improvements regarding STL usage or would you also like general hints and corrections?
    – Ben Steffan
    Jan 5 at 20:08










  • @BenSteffan General hints and suggestions are good, but I am interested in the STL usage. I'm aware that I could improve my naming conventions, the use of Camel Case versus First Cap is inconsistent. Feel free to review everything since it is a learning experience no matter what.
    – pacmaninbw
    Jan 5 at 20:14






  • 1




    Fine, then I'll go ahead.
    – Ben Steffan
    Jan 5 at 20:15










  • Does your program actually compile? A conforming compiler should reject it.
    – Ben Steffan
    Jan 5 at 21:14






  • 1




    I think I remember that Visual Studio allowed that one to slip. Give me 5 minutes, I'm nearly done with my answer that includes a description of what I am talking about.
    – Ben Steffan
    Jan 5 at 21:32












up vote
4
down vote

favorite









up vote
4
down vote

favorite











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 << "...nn" << 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_creditscredits

m_NumberGenerator = new RandomNumber();


std::vector<int> kenoGamePlay::QuickPickNumbers()
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 numbersn";

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()

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 lostn";

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++;









share|improve this 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 << "...nn" << 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_creditscredits

m_NumberGenerator = new RandomNumber();


std::vector<int> kenoGamePlay::QuickPickNumbers()
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 numbersn";

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()

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 lostn";

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++;











share|improve this question












share|improve this question




share|improve this question








edited Jan 5 at 20:21
























asked Jan 5 at 18:31









pacmaninbw

4,99321436




4,99321436











  • Just to make sure, are you only interested in improvements regarding STL usage or would you also like general hints and corrections?
    – Ben Steffan
    Jan 5 at 20:08










  • @BenSteffan General hints and suggestions are good, but I am interested in the STL usage. I'm aware that I could improve my naming conventions, the use of Camel Case versus First Cap is inconsistent. Feel free to review everything since it is a learning experience no matter what.
    – pacmaninbw
    Jan 5 at 20:14






  • 1




    Fine, then I'll go ahead.
    – Ben Steffan
    Jan 5 at 20:15










  • Does your program actually compile? A conforming compiler should reject it.
    – Ben Steffan
    Jan 5 at 21:14






  • 1




    I think I remember that Visual Studio allowed that one to slip. Give me 5 minutes, I'm nearly done with my answer that includes a description of what I am talking about.
    – Ben Steffan
    Jan 5 at 21:32
















  • Just to make sure, are you only interested in improvements regarding STL usage or would you also like general hints and corrections?
    – Ben Steffan
    Jan 5 at 20:08










  • @BenSteffan General hints and suggestions are good, but I am interested in the STL usage. I'm aware that I could improve my naming conventions, the use of Camel Case versus First Cap is inconsistent. Feel free to review everything since it is a learning experience no matter what.
    – pacmaninbw
    Jan 5 at 20:14






  • 1




    Fine, then I'll go ahead.
    – Ben Steffan
    Jan 5 at 20:15










  • Does your program actually compile? A conforming compiler should reject it.
    – Ben Steffan
    Jan 5 at 21:14






  • 1




    I think I remember that Visual Studio allowed that one to slip. Give me 5 minutes, I'm nearly done with my answer that includes a description of what I am talking about.
    – Ben Steffan
    Jan 5 at 21:32















Just to make sure, are you only interested in improvements regarding STL usage or would you also like general hints and corrections?
– Ben Steffan
Jan 5 at 20:08




Just to make sure, are you only interested in improvements regarding STL usage or would you also like general hints and corrections?
– Ben Steffan
Jan 5 at 20:08












@BenSteffan General hints and suggestions are good, but I am interested in the STL usage. I'm aware that I could improve my naming conventions, the use of Camel Case versus First Cap is inconsistent. Feel free to review everything since it is a learning experience no matter what.
– pacmaninbw
Jan 5 at 20:14




@BenSteffan General hints and suggestions are good, but I am interested in the STL usage. I'm aware that I could improve my naming conventions, the use of Camel Case versus First Cap is inconsistent. Feel free to review everything since it is a learning experience no matter what.
– pacmaninbw
Jan 5 at 20:14




1




1




Fine, then I'll go ahead.
– Ben Steffan
Jan 5 at 20:15




Fine, then I'll go ahead.
– Ben Steffan
Jan 5 at 20:15












Does your program actually compile? A conforming compiler should reject it.
– Ben Steffan
Jan 5 at 21:14




Does your program actually compile? A conforming compiler should reject it.
– Ben Steffan
Jan 5 at 21:14




1




1




I think I remember that Visual Studio allowed that one to slip. Give me 5 minutes, I'm nearly done with my answer that includes a description of what I am talking about.
– Ben Steffan
Jan 5 at 21:32




I think I remember that Visual Studio allowed that one to slip. Give me 5 minutes, I'm nearly done with my answer that includes a description of what I am talking about.
– Ben Steffan
Jan 5 at 21:32










1 Answer
1






active

oldest

votes

















up vote
1
down vote



accepted










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!






share|improve this answer

















  • 1




    Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
    – pacmaninbw
    Jan 5 at 22:14











  • @pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
    – Ben Steffan
    Jan 5 at 22:21










  • They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
    – pacmaninbw
    Jan 5 at 22:24










  • @BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
    – Incomputable
    Jan 6 at 11:57










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f184387%2fa-simple-console-based-keno-lottery-game-in-c%23new-answer', 'question_page');

);

Post as a guest






























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
1
down vote



accepted










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!






share|improve this answer

















  • 1




    Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
    – pacmaninbw
    Jan 5 at 22:14











  • @pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
    – Ben Steffan
    Jan 5 at 22:21










  • They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
    – pacmaninbw
    Jan 5 at 22:24










  • @BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
    – Incomputable
    Jan 6 at 11:57














up vote
1
down vote



accepted










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!






share|improve this answer

















  • 1




    Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
    – pacmaninbw
    Jan 5 at 22:14











  • @pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
    – Ben Steffan
    Jan 5 at 22:21










  • They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
    – pacmaninbw
    Jan 5 at 22:24










  • @BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
    – Incomputable
    Jan 6 at 11:57












up vote
1
down vote



accepted







up vote
1
down vote



accepted






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!






share|improve this answer













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!







share|improve this answer













share|improve this answer



share|improve this answer











answered Jan 5 at 21:45









Ben Steffan

4,85011234




4,85011234







  • 1




    Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
    – pacmaninbw
    Jan 5 at 22:14











  • @pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
    – Ben Steffan
    Jan 5 at 22:21










  • They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
    – pacmaninbw
    Jan 5 at 22:24










  • @BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
    – Incomputable
    Jan 6 at 11:57












  • 1




    Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
    – pacmaninbw
    Jan 5 at 22:14











  • @pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
    – Ben Steffan
    Jan 5 at 22:21










  • They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
    – pacmaninbw
    Jan 5 at 22:24










  • @BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
    – Incomputable
    Jan 6 at 11:57







1




1




Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
– pacmaninbw
Jan 5 at 22:14





Thanks, and LOL! FYI, the reference is my fault because I didn't catch it in the OP's question.
– pacmaninbw
Jan 5 at 22:14













@pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
– Ben Steffan
Jan 5 at 22:21




@pacmaninbw You're welcome! I'm still very much astonished that MSVC doesn't reject that code.
– Ben Steffan
Jan 5 at 22:21












They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
– pacmaninbw
Jan 5 at 22:24




They must have been trying to placate PHP programmers that were forced to switch to C++. Note to self, copy code to Linux and compile with gcc or g++ before posting question.
– pacmaninbw
Jan 5 at 22:24












@BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
– Incomputable
Jan 6 at 11:57




@BenSteffan, I had something similar but I didn't get even a warning on clang. When I approach things like this, I always ban constructors taking type&& and const type&&. There are better techniques described in this paper.
– Incomputable
Jan 6 at 11:57












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f184387%2fa-simple-console-based-keno-lottery-game-in-c%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Greedy Best First Search implementation in Rust

Function to Return a JSON Like Objects Using VBA Collections and Arrays

C++11 CLH Lock Implementation