Write a program that simulates a lottery

Question

I am taking a C++ course and we have received our final assignment, I was looking for some suggestions and or tips so I can continue to improve.

I am aware that the use of using namespace std is bad practice but my professor prefers that we use this way since it is a beginners course.

The assignment was a two part assignment I have commented out the piece of the code that was unneeded for the second part.

Part One:

Write a program that simulates a lottery. The program should have an array of 7 integers named winningDigits, with a randomly generated number in the range of 0 through 9 for each element in the array. The program should ask the user to enter 7 digits and should store them in a second integer array named player. The program must compare the corresponding elements in the two arrays and count how many digits match.

For example, the following shows the winningDigits array and the player array with sample numbers stored in each. There are two matching digits, elements 2 and 4.

WinningDigits: 7 4 9 1 3 5 0

Player: 4 2 9 7 3 4 2

Part Two:

Modify the lab solution so that the player numbers are entered not from the keyboard, but from the file player.dat (you will need to create this file and add 7 numbers).

What can I do better?

#include <iostream>
#include <fstream>
using namespace std;

int main()
{
    const int size = 7;
    int winningDigits[size];
    int player[size];
    int count=0;
    ifstream datafile;  

    datafile.open("player.dat");

    for(int i=0; i<size; i++)
    {
        winningDigits[i]=rand()%10;
        datafile >> player[i];
        if (winningDigits[i]==player[i])
        count++;
    }
/*
for(int i=0; i<size; i++)
{
    winningDigits[i]=rand()%10;
    cout << "Enter number " << i+1 << " ";
    cin >> player[i];

    if (winningDigits[i]==player[i])
        count++;
}
*/
    cout << "WinningDigits: ";
    for(int i=0; i<size; i++)
    cout << winningDigits[i] << " ";

    cout << "\nPlayer:        ";
    for(int i=0; i<size; i++)
    cout << player[i] << " ";

    cout << " \nThere are " << count << " matching elements ";
    for(int i=0; i<size; i++)
    if(winningDigits[i]==player[i])
    cout << i+1 << " ";
    cout << "\n";
}

Answer 1

There's quite a few things I would change here. Getting quality random number generation right is difficult, (these questions come up a lot so it's something I've blogged about), but thankfully with the newer c++11 standard things are a fair bit easier.

Program Correctness

Making sure that your program is correct is extremely important. As it stands this program is not correct (as in does not meet the specification):

cout << " \nThere are " << count << " matching elements ";
for(int i=0; i<size; i++){
    if(winningDigits[i]==player[i]){
        cout << i+1 << " ";
    }
}
cout << "\n";

This comparison assumes that the arrays are sorted, which is not the case. So matching elements could be completely missed here. Sort the arrays first before comparing them or consider a different data structure to store the results from generating the numbers.

Basic things:

There's commented out code in the source, this is almost always a bad sign. Keep code changes tracked by version control, not commented out blocks.

Loops like this are a nightmare formatting wise:

for(int i=0; i<size; i++)
cout << winningDigits[i] << " ";

I personally always use braces (as does the coding standard we go by at my work) but whatever you do you need a tab here:

for(int i=0; i<size; i++){
    cout << winningDigits[i] << " ";
}

Another thing is this:

using namespace std;

Other people have commented on this, but generally speaking the only time you would really want to do this is when you are making some small throwaway program for testing a concept or making an example or similar. You probably don't want to do it in any production code. This pollutes the namespace which is something you don't want, see this https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice for more info on that.

If you want to cut down on on typing you can bring in just the names you need by doing:

using std::cout;
using std::cin;

an so on.

Standard library containers

Generally speaking with c++ you don't want to use raw arrays, the standard container classes are safer and allow you to be more productive. There's a c++ FAQ article that deals with just this, so give that a read.

int winningDigits[size];
int player[size];

Instead of these I would use std::array for both of these as the length is both fixed and known at compile time. Were then lengths not known then std::vector would most likely be the best choice. These become:

#include <array> //header needed for std::array

std::array<int, size> winningDigits;
std::array<int, size> player;

Now this ends up being the same type in a bunch of different places so a typedef will help make things easier:

typedef std::array<int, size> lottery_results_t;
lottery_results_t winningDigits;
lottery_results_t player;

An additional benefit of the typedef is that if you do change the type later it will be easier to do so.

Random number generation

rand() is a really bad random number generator, to the point where the standard committee actually made a report about depreciating it. Additionally rand() gets worse when you take the modulus of it (statistically speaking).

We can avoid all these problems by just using the newer c++ random number library in a manner like this:

std::default_random_engine generator;
generator.seed( time(0) );
std::uniform_int_distribution<int> distribution(0,9);//note the min and max parameters are inclusive here
while(true)
{
    std::cout << distribution(generator) << std::endl;
}

Demonstration here: http://ideone.com/xzgcZy

The output code definitely violates the "don't repeat yourself" principle. Success200 has posted a great answer that outlines some of the things you can do to reduce that duplication.

Answer 2

Randomness

It's not a very good lottery if the numbers that are drawn are identical every time. Before you call std::rand(), you need to seed pseudorandom number generator by calling std:srand() once. Typically, you use the current time as a seed value. (Seeding it again won't make it more random.)

#include <cstdlib>
#include <ctime>

int main()
{
    std::srand(std:time(0));
    …
}

Answer 3

Abstraction

I noticed that there is a chunk of code that was commented out when you implemented Part Two. A more elegant way to alter the behaviour of the program slightly is to think of the user input abstractly as a source of digits, and create an two kinds of objects to represent them. That way, the loop does not need to change just because your input source changes.

class DigitSource {
  public:
    virtual DigitSource &operator>>(int &) = 0;
};

class ConsoleDigitSource : public DigitSource {
  public:
    ConsoleDigitSource() : i(0) {}
    virtual DigitSource &operator>>(int &num) {
        std::cout << "Enter number " << ++i << " ";
        std::cin >> num;
        return *this;
    }

  private:
    int i;
};

class FileDigitSource : public DigitSource {
  public:
    FileDigitSource(std::ifstream &datafile) : file(datafile) {}
    virtual DigitSource &operator>>(int &num) {
        file >> num;
        return *this;
    }

  private:
    std::ifstream &file;
};

int main()
{
    …

    ifstream datafile("player.dat");
    FileDigitSource choices(datafile);

    for(int i=0; i<size; i++)
    {
        winningDigits[i]=rand()%10;
        choices >> player[i];
        if (winningDigits[i]==player[i]) count++;
    }

    …
}

To restore the old keyboard-prompting behaviour, simply change two lines of main() to:

    ConsoleDigitSource choices;

For that matter, you could also create a RandomDigitSource that represents the lottery draw.

class RandomDigitSource : public DigitSource {
  public:
    RandomDigitSource() { std::srand(std::time(0)); }
    virtual DigitSource &operator>>(int &num) {
        num = std::rand() % 10;
        return *this;
    }
};

int main()
{
    …
    RandomDigitSource lotteryDraw;
    ConsoleDigitSource choices;

    for(int i=0; i<size; i++)
    {
        lotteryDraw >> winningDigits[i];
        choices >> player[i];
        if (winningDigits[i]==player[i]) count++;
    }
    …
 }

Answer 4

Regarding your edit:

One way to avoid std:: littering your code is to import only the functions you are using.

using std::cout;
using std::cin;
using std::foo;

Answer 5

• Remove the commented out code (that's why we have source control so you can retrieve temporary deleted code) because it confuses the reader

• Always put curly brackects around your for loops and indent properly:

  for(int i=0; i<size; i++) {
      cout << winningDigits[i] << " ";}
  

Answer 6

Something no one else has commented on is variable naming.

ifstream datafile;  
datafile.open("player.dat");

This naming doesn't really tell me anything. The filename itself is somewhat misleading, too, as I read that to be "data associated with a player" and not "one players data input." What is the datafile? You end up using

I would rename the ifstream into something which conveys the meaning more fully than datafile, something like:

ifstream playerLotteryNumberFile;  
playerLotteryNumberFile.open("player.dat");

then likewise, modify:

int player[size];

to be something more like:

int playerLotteryNumbers[size];

You could also remove Lottery from both.

But this means later when you do your comparisons and printing it will make a lot more sense.

    if (winningDigits[i]==player[i])
    count++;

I read this and wonder why you are comparing a digit to a player (ie person). But the following reads a lot more smoothly:

    playerLotteryNumberFile >> playerLotteryNumbers[i];
    if (winningDigits[i]==playerLotteryNumbers[i]) {
        count++;
    }

Doing this sort of thing means you can more easily read your code on a line by line basis without referring to "what was player? what was in datafile?"