### Hello, I'd appreaciate a code review!

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! :)

 ``123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127128129130131132133134135136137138139140`` ``````#include #include #include #include using namespace std; void PrimaryNums(); void BonusNums(); void PrintNums(); vector CheckPrimary(vector &pNums); vector CheckBonus(vector &bNums); vector FillPrimary(vector &pNums); vector FillBonus(vector &bNums); int main() { PrintNums(); return 0; } void PrimaryNums() { vector 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 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 CheckPrimary(vector &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 CheckBonus(vector &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 FillPrimary(vector &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 FillBonus(vector &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"; }``````
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/34250914/issue-with-using-namespace-std

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.
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.
It's easier to use a std::set as these can't hold duplicate values. Possibly:

 ``1234567891011121314151617181920212223242526272829303132333435363738394041424344454647484950`` ``````#include #include #include void PrimaryNums(); void BonusNums(); void PrintNums(); std::mt19937 rng(std::random_device {}()); int main() { PrintNums(); } void PrimaryNums() { std::uniform_int_distribution distrib(1, 50); std::set 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 distrib(1, 12); std::set 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'; }``````

Or even simpler:

 ``123456789101112131415161718192021222324252627282930313233`` ``````#include #include #include std::set getNums(unsigned high, unsigned req); void printNums(const std::set&); 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 getNums(unsigned high, unsigned req) { std::uniform_int_distribution distrib(1, high); std::set prim; while (prim.size() != req) prim.insert(distrib(rng)); return prim; } void printNums(const std::set& nums) { for (auto a : nums) std::cout << a << ' '; std::cout << '\n'; }``````

Thanks for the tips guys, i'll definitely try out all you mentioned, i appreciate the help!
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.

 ``12345`` ``````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.
 ``1234567891011121314151617181920212223242526272829303132333435363738394041424344454647484950515253545556575859606162636465666768697071727374757677787980`` ``````// Generate and print 5 primary and 2 bonus number for a lottery #include #include #include #include using namespace std; void PrimaryNums(); void BonusNums(); void RemoveDuplicates(vector &pNums); void Fill(vector &pNums, unsigned sz); void Print(vector &nums); int main() { srand(time(0)); PrimaryNums(); cout << "\n\n"; BonusNums(); cout <<"\n"; return 0; } // Print the vector void Print(vector &nums) { for( auto a : nums) cout << a << " "; cout << endl; } // Generate and print 5 primary numbers void PrimaryNums() { vector pnums; Fill(pnums, 5); cout << "Your lucky primary numbers are: "; Print(pnums); } // Generate and print 2 bonus numbers void BonusNums() { vector bnums = {}; Fill(bnums, 2); cout << "Your lucky bonus numbers are: "; Print(bnums); } // Remove duplicates from pNums void RemoveDuplicates(vector &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 &pNums, unsigned sz) { do { while(pNums.size() < sz) { int random = rand () % 50 + 1; pNums.push_back(random); } RemoveDuplicates(pNums); } while (pNums.size() < sz); }``````

 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.