Improve Beginner Code

[Majeek]

So I'm pretty new to C++ and after making this little game after I finished the 1st section of my lessons. (Section 2 is moving onto Unreal Engine) I wanted to know what I can improve and work on so I can get the basics down? (The Game is Bulls and Cows where you have to guess the 4 digit isogram.)

#include <iostream> 
#include <string> 
#include <vector> 
#include <sstream> 
#include <time.h>
#include <cstdlib>
#include <ctime>

void PlayGame();
std::vector <char> Ask();
void HiddenNumberGenerator();
void Logic(std::vector <char> GuessVector);
void Pause();

std::vector <char> HiddenNumber;

std::string IntToString(const int& TheInt) { 
	std::stringstream GuessSString;
	GuessSString << TheInt;
	std::string GuessString = GuessSString.str(); 
	return GuessString;
}

std::vector <char> StringToVector(const std::string &TheString) { 
	std::vector <char> GuessVector;

	for (size_t i = 0; i < TheString.length(); i++) { 
		GuessVector.push_back(TheString[i]);
	}
	return GuessVector;
}

bool CheckNumbers(const std::vector <char> &TheVector) {
	for (size_t i = 0; i < TheVector.size(); i++) {
		for (size_t j = 0; j < TheVector.size(); j++) {
			if (TheVector[i] == TheVector[j] && i != j) {
				return false;
			}
		}
	}
	return true;
}

int CheckBulls(const std::vector <char> &TheVector) {
	int Bulls = 0;
	for (size_t i = 0; i < TheVector.size(); i++) {
		if (TheVector[i] == HiddenNumber[i]) {
			Bulls += 1;
		}
	}
	return Bulls;
}

int CheckCows(const std::vector <char> &TheVector) {
	int Cows = 0;
	for (size_t i = 0; i < TheVector.size(); i++) {
		for (size_t j = 0; j < TheVector.size(); j++) {
			if (TheVector[i] == HiddenNumber[j] && i != j) {
				Cows += 1;
			}
		}
	}
	return Cows;
}

int main() {
	srand((unsigned int)time(NULL));
	std::cout << "Rules of Bulls and Cows: \n-Cant have repeated numbers. \n-Can only be 4 digits. \n-Is a number.\n\n\n";

	HiddenNumberGenerator();
	PlayGame();
	return 0;
}

void PlayGame() {
	std::vector <char> GuessVector = Ask();
	Logic(GuessVector);
	PlayGame();
}

std::vector <char> Ask() {
	int Guess;

	while (true) {
		std::cout << "Guess the 4 digit isogram: ";
		std::cin >> Guess;
		std::cin.clear();
		std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

		if (Guess > 9999 || Guess < 1000) {
			std::cout << "Incorrect syntax\n\n";
		}
		else {
			break;
		}
	}

	std::string GuessString = IntToString(Guess); 

	std::vector <char> GuessVector = StringToVector(GuessString); 

	if (CheckNumbers(GuessVector) == false) {
		std::cout << "Not an isogram.\n\n";
		Ask();
	}
	return GuessVector;
}

void HiddenNumberGenerator() {
	int HiddenNumberInt = rand() % 9000 + 1000;
	int Error = 0;

	std::string HiddenNumberString = std::to_string(HiddenNumberInt);
	HiddenNumber = StringToVector(HiddenNumberString);

	for (size_t i = 0; i < HiddenNumber.size(); i++) {
		for (size_t j = 0; j < HiddenNumber.size(); j++) {
			if (HiddenNumber[i] == HiddenNumber[j] && i != j) {
				Error += 1;
			}
			else if ((i == 3) && (j == 3) && (Error > 0)) {
				HiddenNumberGenerator();
			}
		}
	}
}

void Logic(std::vector <char> GuessVector) {
	int Bulls = CheckBulls(GuessVector);
	int Cows = CheckCows(GuessVector);

	if (Bulls == 4) {
		std::cout << "You won the game!";
		Pause();
		exit(0);
	}
	else {
		std::cout << "Bulls: " << Bulls;
		std::cout << "\nCows: " << Cows << "\n\n";
	}
}

void Pause() {
	std::cout << "\n\n";
	system("PAUSE");
}

[ne555]

//function Ask()
	std::vector <char> GuessVector = StringToVector(GuessString); 

	if (CheckNumbers(GuessVector) == false) {
		std::cout << "Not an isogram.\n\n";
		Ask(); //discard the returned value
	}
	return GuessVector; //return the old value 

in case the user's input is invalid, you do ask them again but you return the old one
to clarify, here's an example run

$ gdb a.out
(gdb) break Logic(std::vector<char, std::allocator<char> >)
(gdb) run
...
Guess the 4 digit isogram: 1111
Not an isogram.

Guess the 4 digit isogram: 1234

Breakpoint 1, Logic (GuessVector=std::vector of length 4, capacity 4 = {...}) at foo.cpp:129
129             int Bulls = CheckBulls(GuessVector);
(gdb) print GuessVector
$1 = std::vector of length 4, capacity 4 = {49 '1', 49 '1', 49 '1', 49 '1'}

notice that Logic gets the invalid {1,1,1,1} vector
I don't understand why you launch a recursive call instead of simply looping (same thing you are doing in `PlayGame()' and `HiddenNumberGenerator()')

some other things in no particular order:
- ¿why is Logic() in charge of program termination? (I had to search for exit() in order to find this)
- don't make HiddenNumber a global variable
- comment your code (make it self-commenting). In CheckNumbers() you are checking repetitions, should know that at first glance (sure, can read the code, ¿but is your code correct?)
- variable names should be meaningful in the context, ¿why in StringToVector() the result vector is called GuessVector?
- seems that HiddenNumberGenerator() is checking repeated elements, ¿why don't simply call to CheckNumbers()? (bonus, ¿can you guess why `Error' is always even?)
- in the condition in line 121 you are checking to see if that's the last iteration of the loop, ¿why isn't simply outside the loop?
- you have IntToStrig() and then StringToVector(), may do IntToVector() ¿but why so much yo-yo code?

[jonnin]

some stuff you might look at. None of it is 'wrong' just could be better:

#include <time.h> take this out. <ctime> is the c++ version and you have it too.

std::vector <char> Ask();
std::vector <char> HiddenNumber;

global variables cause problems. Try to avoid using them -- allocate this in main, and pass it to the functions that need it as a parameter. Depending on what you do, vector char vs string … one may or may not be more useful.

checknumbers is inefficient. can you think of a better way? What if you sorted it, would identical elements be next to each other...?


consider using <random> instead of C's tools.

[ne555]

> checknumbers is inefficient. can you think of a better way? What if you sorted it,
i don't think that it justify for four digits
besides, as the order matters, you'll need to copy the vector.

[jonnin]

good point. I was initially thinking it might be run for a large problem one day, but in hindsight, it won't ever get THAT big.

[keskiverto]

Lets look at HiddenNumberGenerator. You want four unique digits, where the first digit is not 0 (because 0xyz < 1000).

What do you do there?
You pick a random number from [1000-9999].
First digit is not 0, but you are quite likely to get a number where all digits are not unique.
You solve that by trying repeatedly, until you get lucky.


If you have a shuffled deck of ten unique cards and you pull four cards from that deck, then you know that all four cards are unique. Luck is not involved.
The only issue is that the first pulled card could be 0, but you do know that there are 9 cards remaining and none of them is 0 if the first was 0.

For example:

#include <iostream>     // std::cout
#include <algorithm>    // std::shuffle
#include <string>
#include <random>       // std::default_random_engine
#include <chrono>       // std::chrono::system_clock

std::string HiddenNumberGenerator()
{
  std::string digits = "0123456789"; // a deck of 10 unique cards
  unsigned seed = std::chrono::system_clock::now().time_since_epoch().count();
  std::shuffle( digits.begin(), digits.end(), std::mt19937(seed) ); // in random order

  // pull four and make sure that the first is not 0
  if ( digits[0] == '0' ) return digits.substr( 1, 4 );
  else return digits.substr( 0, 4 );
}

int main () {
  for ( int g = 0; g < 10; ++g ) std::cout << HiddenNumberGenerator() << '\n';
}

Lets look at:
std::string IntToString(const int& TheInt);
Then look at line 113:
std::string HiddenNumberString = std::to_string( HiddenNumberInt );
Having two functions, std::to_string and IntToString for same task is redundant.
Prefer the standard library version.