Simple program code review request

I just finished the tutorial so I've been going through some beginner c++ exercises. The objective was to create a program that guesses your number using "higher" and "lower" commands.

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
// Guess My Number
#include <iostream>

bool guessed{false};
int guesses{0};
int minNumber{1};
int maxNumber{101}; // Initialized at 101 because guessNumber can't guess higher than maxNumber - 1
int cpuGuess{50}; // As good a place to start as any

void guessNumber(char, int);

int main() {
	char response;

	std::cout << "Pick a number between 1 and 100 and I'll guess that shit.\n\n";

	do {
		std::cout << "Is your number: " << cpuGuess << '\n';
		std::cout << "Correct(c), higher(h) or lower(l)? ";
		std::cin >> response;

		switch(response) {
			case 'c':
				guesses++;
				std::cout << "I'm the best I got it right in " << guesses;
				guesses == 1 ? std::cout << " guess." : std::cout << " guesses."; // I had to
				guessed = !guessed;
				break;
			case 'h':
			case 'l':
				guesses++;
				guessNumber(response, cpuGuess);
				break;
			case 'q':
				guessed = !guessed;
				break;
			default:
				std::cout << "Enter c, h or l.\n";
				break;
		}

	} while(!guessed);

	return 0;
}

void guessNumber(char c, int i) {
	int newGuess;

	c == 'h' ? minNumber = i : maxNumber = i;
	newGuess = (maxNumber - minNumber) / 2 + minNumber;

	if (newGuess == cpuGuess) {
		std::cout << "Looks like I guessed your number in " << guesses << " tries.\n";
		std::cout << "Next time don't lie to me.\n"; // Needs more passive aggressiveness
		guessed = !guessed;
	} else {
		cpuGuess = newGuess;
	}
}


It's a simple exercise but it'd be great if you can tell me if I'm doing anything stupid or terribly wrong. Like the scope of most of those variables. Is it better to put them in main and pass them by reference?

Anyway, thanks for your time
Last edited on
Don't worry about the scopes of the variables at the beginning, you'll learn it early enough when globals are good and when they are bad.
For this example it is okey to have them in global scope.

I'd say it's pretty good and easy to read. (i didn't test if it works)
The only thing that was hard to read (because of the syntax) are the parts if?then:else.
In these cases you should group your statements, I had a hart time finging the : on the first glance.
c == 'h' ? minNumber = i : maxNumber = i;

c == 'h' ? minNumber=i : maxNumber=i;

Last edited on
Globals are bad most of the time. I wouldn't advise anyone to use them even in simple programs like this. Why encourage bad habits?
because he might prevent himself from writing globals all together and he might get obsessed with only declaring local variables.

This can result in unnecessary complicated code to just get a variable in local scope.
(You have to add a pass-by-reference-parameter for all functions and that have access to the global variables)

I think it's not that important at the beginning and in this example i think that globals are perfectly fine.
The only thing that was hard to read (because of the syntax) are the parts if?then:else.
In these cases you should group your statements, I had a hart time finging the : on the first glance.
c == 'h' ? minNumber = i : maxNumber = i;

c == 'h' ? minNumber=i : maxNumber=i;


I'm trying to develop a consistent coding style and the spaces around operators are my biggest issue. With the example you provided and with for statements I don't want to use spaces but everywhere else I do. For some reason my brain won't let me mix and match.

Globals are bad most of the time. I wouldn't advise anyone to use them even in simple programs like this. Why encourage bad habits?


That's what I was afraid of. In the tutorial there were some examples that had unnecessary global scope variables and I always moved them into main. I don't think I'd ever not use them but looking back they didn't feel necessary in this program.

because he might prevent himself from writing globals all together and he might get obsessed with only declaring local variables.

This can result in unnecessary complicated code to just get a variable in local scope.
(You have to add a pass-by-reference-parameter for all functions and that have access to the global variables)


annnd that's what I just finished doing

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
// Guess My Number
#include <iostream>

int guessNumber(char, int, int&, int&);

int main() {
	char response;
	bool guessed{false};
	int guesses{0};
	int minGuess{1};
	int maxGuess{101}; // Initialized at 101 because guessNumber can't guess higher than maxNumber - 1
	int cpuGuess{50}; // As good a place to start as any
	int newGuess;

	std::cout << "Pick a number between 1 and 100 and I'll guess that shit.\n\n";

	do {
		std::cout << "Is your number: " << cpuGuess << '\n';
		std::cout << "Correct(c), higher(h) or lower(l)? ";
		std::cin >> response;

		switch(response) {
			case 'c':
				guesses++;
				guessed = !guessed;

				std::cout << "I'm the best I got it right in " << guesses;
				guesses == 1 ? std::cout << " guess." : std::cout << " guesses.";
				break;
			case 'h':
			case 'l':
				guesses++;
				newGuess = guessNumber(response, cpuGuess, minGuess, maxGuess);
				if (newGuess == cpuGuess) { // If the number has already been gussed
					std::cout << "Looks like I guessed your number in " << guesses << " tries.\n";
					guessed = !guessed;
				} else {
					cpuGuess = newGuess;
				}
				break;
			case 'q':
				guessed = !guessed;
				break;
			default:
				std::cout << "Enter c, h or l.\n";
				break;
		}

	} while(!guessed);

	return 0;
}

int guessNumber(char g, int cpuG, int& minG, int& maxG) {
	int newG;

	g == 'h' ? minG = cpuG : maxG = cpuG;
	newG = (maxG - minG) / 2 + minG;

	return newG;
}


I moved all the global variables into main, changed guessNumber to return an int and passed it some variables by reference. The change of return type was because I felt the if statement didn't belong in the function. It went beyond just guessing a number.

I'm happier with it now although I'm still on the fence about the <condition> ? <a> : <b> lines and what they do to the readability of the code. Maybe I'll make an exception on the spacing formatting
Last edited on
Maybe I'll make an exception on the spacing formatting
You could put braces around them
c == ('h') ? (minNumber = i) : (maxNumber = i);

annnd that's what I just finished doing
nice, you're quite a motivated one

I'm trying to develop a consistent coding style and the spaces around operators are my biggest issue. With the example you provided and with for statements I don't want to use spaces but everywhere else I do. For some reason my brain won't let me mix and match.
I use c++ since 3 years and my coding style changes constantly (and I don't want to touch most of the old code anymore)
I wish you good luck, I didn't succeed in it (but I think I am pretty close)

That's what I was afraid of. In the tutorial there were some examples that had unnecessary global scope variables and I always moved them into main. I don't think I'd ever not use them but looking back they didn't feel necessary in this program.
That's a good idea, exspecially when having read/write access from everywhere.
Last edited on
You could put braces around them


That works! I tried parentheses around the condition then curly braces around the statements but that wouldn't compile. I'll go with three sets of parentheses. It makes it immediately obvious what's going on.

Thanks for your help!
If that's not enough (for long statements) you could write those in 3 different lines AND have the brackets:
1
2
3
c == ('h') ? 
     (minNumber = i) : 
     (maxNumber = i);
Last edited on
Topic archived. No new replies allowed.