Criticism

[sari]

Hello All,

I am currently learning C++ after about a year of Java and would like some criticism (try not to be to harsh :) ). I wrote a small random guessing game for the command line. Here is the source, also the if statement in the check answer function could probably be shortened anyone know how to do that?

#include <stdlib.h>
#include <iostream>
#include <string>

using namespace std;

void won() {
	cout << "Congradulations You have won!!!" << endl << "Do you want to play again?> ";
	string pg;
	cin >> pg;
	if (pg != "y") {
		exit(0);
	}
}
	

bool check_ans(int gu, int c) {
	if (gu <= c + 3 && gu >= c - 3) {
		return true;
	}else if (gu == 51) { //leave the game
		cout << "\nBye!!!\n";
		exit(0);
	} else {
		 if (gu > c) {
			cout << "You went too high\n";
		} else {
			cout << "You are too low!\n";
		}
		return false;
	}
}

int main() {
	int g; //guess variable
	bool hasWon = false;
	srand(time(NULL)); //start the random seed
	cout << "Welcome to the random game" << endl << "you have three guess's at the correct answer" << endl; //print opening
	cout << "Your answer must be within three of the correct answer" << endl << "The answer will range from 1 to 50\n"; //print opening (still)
	while (!hasWon) {
		int ran= rand() % 50 + 1; //initialize the random number
		cout<< ran << "\nYour first guess> ";
		cin >> g;
		hasWon = check_ans(g, ran);
		if (hasWon) {
			won();
			hasWon = false;
			continue;
		}
		cout << "Your second guess> ";
		cin >> g;
		hasWon = check_ans(g, ran);
		if (hasWon) {
			won();
			hasWon = false;
			continue;
		}
		cout << "Your third guess> ";
		cin >> g;
		hasWon = check_ans(g, ran);
		if (hasWon) {
			won();
			hasWon = false;
			continue;
		}
		
		cout << "The correct answer was " << ran << endl;
	}
	return 0;
}

[jsmith]

You have essentially copy-pasted the same block of code (lines 28-41) three times. This should suggest that a loop would be more appropriate.

abs( x - y ) <= 3

is true if x and y are within 3 of each other.

[rossipoo]

You don't need to check every case manually:
if (g <= c + 3 && g >= c - 3)

You have a while loop, which I agree might be useful, but you aren't using it at all right now. Notice that your code to ask for input and congradulate (or not) is almost exactly the same each time. A while loop, or a do-while loop would be useful to save having to type the code three times. Try to solve the problem, if you gave the user an unlimited number of guesses.

[sari]

Ok thank you all for that I think that I see what can be improved/changed I'll fix it and repost

[sari]

I have edited the source I forgot to put in the rules that the user has only three chances :)

[rossipoo]

int guessCount = 0;
do {
    ++guessCount;
    // here goes code that checks one guess only
} while (guessCount < 3  &&  !check_ans(g, ran) );

[sari]

Ah, Simple logical error,

Thanks