C++

Forum

 
Hello, I'd appreaciate a code review!

HelpMePleaseTy (2)
As the title says, i'd like to ask for a code review. The programs works as i imagined, but i'd like to improve it, if i can in any way. The program helps you pick out numbers for lottery. In case of primary numbers, it pushes 5 numbers in a vector, checks for duplicates, deletes them if they're present, and fills the vector back up. It does the same for bonus numbers, except it's only 2 bonus numbers. And then it prints the numbers. Thank you in advance! :)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
 
#include <iostream>
#include <time.h>
#include <vector>
#include <algorithm>

using namespace std;

void  PrimaryNums();
void  BonusNums();
void PrintNums();
vector <int> CheckPrimary(vector <int> &pNums);
vector <int> CheckBonus(vector <int> &bNums);
vector <int> FillPrimary(vector <int> &pNums);
vector <int> FillBonus(vector <int> &bNums);

int main()
{
    PrintNums();
    return 0;
}

void PrimaryNums()
{
   
    vector <int> pnums;
    srand(time(0));
    
    while(pnums.size() < 5)
    {
        int random = rand() % 50 + 1;
        pnums.push_back(random);
    }
    
    CheckPrimary(pnums);

    if(pnums.size() < 5)
    {
        FillPrimary(pnums);
    }
   
    cout << "Your lucky primary numbers are: ";
    for( auto a : pnums)
        cout << a << " ";
    cout << endl;
}


void BonusNums()
{
  
    vector <int> bnums = {};
    srand(time(0));
    
    while (bnums.size() < 2)
    {
        int random = rand() % 12 + 1;
        bnums.push_back(random);
    }
    
    CheckBonus(bnums);
    
    if(bnums.size() < 2)
    {
        FillBonus(bnums);
    }
    
  
    cout << "\n\nYour lucky bonus numbers are: ";
    for(auto a : bnums)
        cout << a << " ";
   cout << endl;
}


vector <int> CheckPrimary(vector <int> &pNums)
{
    auto end = pNums.end();
    for (auto it = pNums.begin(); it != end; ++it) {
        end = std::remove(it + 1, end, *it);
    }
 
    pNums.erase(end, pNums.end());
    return pNums;
    
}

vector <int> CheckBonus(vector <int> &bNums)
{
    
    auto end = bNums.end();
    for (auto it = bNums.begin(); it != end; ++it) {
        end = std::remove(it + 1, end, *it);
    }
 
    bNums.erase(end, bNums.end());
   
    return bNums;
}


vector <int> FillPrimary(vector <int> &pNums)
{
    
     while(pNums.size() < 5)
    {
            int random = rand () % 50 + 1;
            pNums.push_back(random);
    }
     CheckPrimary(pNums);
    if(pNums.size() < 5)
    {
            FillPrimary(pNums);
    }
    return pNums;
}


vector <int> FillBonus(vector <int> &bNums)
{
     while(bNums.size() < 2)
    {
        int random = rand() % 12 + 1;
        bNums.push_back(random);
    }
    
    CheckBonus(bNums);
    if(bNums.size() < 2)
    {
            FillBonus(bNums);
    }
    return bNums;
}

void PrintNums()
{
    
    PrimaryNums();
    BonusNums();
    cout <<"\n";
}
deleted account xyzzy (5768)
1. Dump using rand/srand. Use the C++ <random> library instead.
http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2013/n3551.pdf

The C random functions have serious problems.
https://web.archive.org/web/20180123103235/http://cpp.indi.frih.net/blog/2014/12/the-bell-has-tolled-for-rand/

2. Stop using namespace std;, when using the C++standard library fully qualify usages. For example std::cout instead of cout.

It isn't "bad", as such, having it can cause problems as your programs grow in size when you write your own functions, etc.
https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice
https://stackoverflow.com/questions/34250914/issue-with-using-namespace-std

3. Consider splitting your single source file into separate source/header files.
https://www.learncpp.com/cpp-tutorial/programs-with-multiple-code-files/

In this particular case 2 source files and 1 header.

Put the function declarations into the header, and the function definitions into a source file. Your main function into the other source file. Make sure you use header guards in your header file, and #include your custom header in both source files.
jonnin (11437)
time.h is a C header. you want <ctime>
but..
rand() is also C. you will want <random> in c++ programs.
random seeding should only happen one time, usually in main, unless you want to repeat a sequence again (in which case, re-seed with the same value to start over).

main does not require return 0. Its fine to put it in.

namespace std is bad practice, consider using smaller parts of std (eg using std::cout) or typing std:: on the calls.

naming conventions. according to main, your program prints some numbers, probably a loop over a vector. No, wait... print nums calls functions that *generate* numbers and do logic and so on. print nums is poorly named, because it does much more than print. If the idea is a one-stop call from main, call it "main_program" or something more meaningful.

a little more advanced, but try to keep I/O distinct from logic/work. If you do that, it is much, much easier to rebuild your program in a UI or some other design (eg file I/O instead of typed) than it is to fix it with embedded I/O in all the main routines.

All in all, its not bad. These kinds of posts are asking for nitpicking, and so you get raked over the coals a bit, but that does not mean you should panic or be frustrated. Honestly, the program works, or looks like it does, and that is a big first step. I would say it is more important to absorb the things I told you for next time than to fix this one, but fixing it may help drive the ideas home if you have the time.
seeplus (6591)
It's easier to use a std::set as these can't hold duplicate values. Possibly:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
 
#include <iostream>
#include <random>
#include <set>

void PrimaryNums();
void BonusNums();
void PrintNums();

std::mt19937 rng(std::random_device {}());

int main() {
	PrintNums();
}

void PrimaryNums() {
	std::uniform_int_distribution<unsigned> distrib(1, 50);
	std::set<unsigned> prim;

	while (prim.size() != 5)
		prim.insert(distrib(rng));

	std::cout << "Your lucky primary numbers are: ";

	for (auto a : prim)
		std::cout << a << " ";

	std::cout << '\n';
}

void BonusNums() {
	std::uniform_int_distribution<unsigned> distrib(1, 12);
	std::set<unsigned> bonus;

	while (bonus.size() != 2)
		bonus.insert(distrib(rng));

	std::cout << "\nYour lucky bonus numbers are: ";

	for (auto a : bonus)
		std::cout << a << " ";

	std::cout << '\n';
}

void PrintNums() {
	PrimaryNums();
	BonusNums();

	std::cout << '\n';
}

seeplus (6591)
Or even simpler:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
 
#include <iostream>
#include <random>
#include <set>

std::set<unsigned> getNums(unsigned high, unsigned req);
void printNums(const std::set<unsigned>&);

std::mt19937 rng(std::random_device {}());

int main() {
	std::cout << "Your lucky primary numbers are: ";
	printNums(getNums(50, 12));

	std::cout << "Your lucky bonus numbers are: ";
	printNums(getNums(12, 2));
}

std::set<unsigned> getNums(unsigned high, unsigned req) {
	std::uniform_int_distribution<unsigned> distrib(1, high);
	std::set<unsigned> prim;

	while (prim.size() != req)
		prim.insert(distrib(rng));

	return prim;
}

void printNums(const std::set<unsigned>& nums) {
	for (auto a : nums)
		std::cout << a << ' ';

	std::cout << '\n';
}

HelpMePleaseTy (2)
Thanks for the tips guys, i'll definitely try out all you mentioned, i appreciate the help!
dhayden (5798)
I think the single most important thing you can do is comment your code. It's hard to know what you're trying to do without them. It doesn't take much, just a sentence or two at the beginning to give an overview of the program and a single sentence or phrase above each function to say what it does.

CheckPrimary, CheckBonus, FillPrimary and FillBonus return a vector that isn't used. They should return void.

Check() is a vague name, and slightly misleading because it does more than just check the vector, it actually changes it. I'd call it RemoveDuplicates() instead because that's more description of what it does.

1
2
3
4
5
 
int main()
{
    PrintNums();
    return 0;
}
Don't do this without a good reason. Put the contents of PrintNum in main instead.

PrimaryNums duplicates much of the logic in FillPrimary.

PrimaryNums and BonusNums are nearly identical. You could reduce these to one function by passing in the desired length of the array. Same comment for CheckPrimary and CheckBonus

BonusNums prints two newlines before the header. Even though you may need those lines, this is the wrong place to print them. They are there because you know the call to BonusNums comes after PrimaryNums, but there's no reason that BonusNums should know or care where it's called. Instead, print the lines in main, after calling PrimaryNums and before calling BonusNums

It doesn't really matter in a small program like this, but it's usually a good idea to separate I/O from computation. In this case, it would be better to have the Fill functions fill the array, but not print it. That makes the functions much more flexible in case you decide to change the program later on.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
 
// Generate and print 5 primary and 2 bonus number for a lottery
#include <iostream>
#include <time.h>
#include <vector>
#include <algorithm>

using namespace std;

void  PrimaryNums();
void  BonusNums();
void RemoveDuplicates(vector <int> &pNums);
void Fill(vector <int> &pNums, unsigned sz);
void Print(vector<int> &nums);

int main()
{
    srand(time(0));
    PrimaryNums();
    cout << "\n\n";
    BonusNums();
    cout <<"\n";
    return 0;
}

// Print the vector
void Print(vector<int> &nums)
{
    for( auto a : nums)
        cout << a << " ";
    cout << endl;
}

// Generate and print 5 primary numbers
void PrimaryNums()
{
   
    vector <int> pnums;
    Fill(pnums, 5);
  
    cout << "Your lucky primary numbers are: ";
    Print(pnums);
}


// Generate and print 2 bonus numbers
void BonusNums()
{
    vector <int> bnums = {};
    Fill(bnums, 2);
  
    cout << "Your lucky bonus numbers are: ";
    Print(bnums);
}


// Remove duplicates from pNums
void RemoveDuplicates(vector <int> &pNums)
{
    
    auto end = pNums.end();
    for (auto it = pNums.begin(); it != end; ++it) {
        end = std::remove(it + 1, end, *it);
    }
 
    pNums.erase(end, pNums.end());
}


// Fill pNums with sz unique numbers
void Fill(vector <int> &pNums, unsigned sz)
{
    do {
	while(pNums.size() < sz)
	    {
		int random = rand () % 50 + 1;
		pNums.push_back(random);
	    }
	RemoveDuplicates(pNums);
    } while (pNums.size() < sz);
}

deleted account xyzzy (5768)
I think the single most important thing you can do is comment your code.

Comment code by using comments, self-document code by using descriptive names for variables and functions.
Topic archived. No new replies allowed.