12
\$\begingroup\$

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";
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 for the discussion of random number generation. We can and should (collectively) do better and one way to do that is to clearly explain what is wrong with the old ways, and how to do it differently and better in the future. \$\endgroup\$ – Edward Dec 5 '14 at 0:48
  • \$\begingroup\$ This is fantastic, I greatly appreciate the suggestions. As for the commented out code, I simply did this for the sake of posting it on this site. I wanted to show both versions in working form. \$\endgroup\$ – Brendon Dec 5 '14 at 2:23
  • 1
    \$\begingroup\$ @user3363195 Glad this has helped. I made an edit to the answer as I noticed that the program that's been posted is not actually correct. \$\endgroup\$ – shuttle87 Dec 5 '14 at 19:12
  • \$\begingroup\$ I am not sure sorting them would be the solution to this issue. It would still compare the players digit that is in the same place as the winningDigits so if the winningDigits are 0 1 2 3 4 5 6 and the player digits are 2 3 4 5 6 7 8 it wouldn't have any matches. I could be misunderstanding you or just not implementing the sorting properly. \$\endgroup\$ – Brendon Dec 6 '14 at 16:06
  • \$\begingroup\$ @user3363195, you are correct, just sorting it isn't quite enough. You need to compare each element if you want to find each match. If it's not sorted you need to search the entire array for each element of the players array which means you always do size comparisons, you can do a bit better with a sorted array. This is a really good example of where a unit test would be useful as you can easily specify the expected result for a given input. Using a unit testing approach would require refactoring (which would be worth doing) as there's currently no functions that the unit tests could call. \$\endgroup\$ – shuttle87 Dec 6 '14 at 16:30
5
\$\begingroup\$

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));
    …
}
\$\endgroup\$
  • \$\begingroup\$ That is a good point. Thank you for the suggestion and I will add it in. \$\endgroup\$ – Brendon Dec 5 '14 at 3:03
4
\$\begingroup\$

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++;
    }
    …
 }
\$\endgroup\$
3
\$\begingroup\$

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;
\$\endgroup\$
  • 1
    \$\begingroup\$ True. But when doing this you should at least try to limit their scope. So do this at the point where you want to use them (so inside the function not at global scope). \$\endgroup\$ – Martin York Dec 4 '14 at 22:40
3
\$\begingroup\$
  • 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] << " ";}
    
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Simon Forsberg Dec 4 '14 at 21:13
  • 1
    \$\begingroup\$ @SimonAndréForsberg This "I am doing it intentionally wrong because I am a beginner"-argument does not sound very convincing to me. That said using std; is just somewhat bad style, it is not terrible. \$\endgroup\$ – nwp Dec 4 '14 at 21:34
  • 2
    \$\begingroup\$ IMHO, it's not necessarily bad to use using namespace std; (it's one of the things that makes namespaces more useable) but it's the abuse of it that is a problem. E.g. using it in every source code file, or (worse) in headers. \$\endgroup\$ – Edward Dec 5 '14 at 0:51
3
\$\begingroup\$

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?"

\$\endgroup\$
  • \$\begingroup\$ Thank you. I definitely should have actually named the file something with meaning. I will make the adjustment in my project. \$\endgroup\$ – Brendon Dec 5 '14 at 2:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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