I just started to learn Java and had to write a "guess the number" game.
Could you please review and give me some feedback?
public static void main(String[] args) {
System.out.println("Welcome to the guess the number game!\n");
System.out.println("Please specify the configuration of the game:\n");
Scanner input = new Scanner(System.in);
System.out.println("Range start number (inclusively):");
int startRange;
startRange = nextValidInt(input);
System.out.println("Range end (inclusively):");
int endRange;
endRange = nextValidInt(input);
System.out.println("Maximum number of attemps:");
int maxAttemp;
maxAttemp = nextValidInt(input);
System.out.println("Your Task is to guess the random number between "
+ startRange + " and " + endRange);
Random randGenerator = new Random();
int randNumber = randGenerator.nextInt((endRange - startRange + 1)
+ startRange);
int numberOfTries = 0;
System.out
.println("You may exit the game by typing; exit - you may now start to guess:");
for (numberOfTries = 0; numberOfTries <= maxAttemp - 1; numberOfTries++) {
int guess;
guess = nextValidInt(input);
if (guess == randNumber) {
System.out.println("Congratz - you have made it!!");
System.out.println("Goodbye");
break;
} else if (guess > randNumber) {
System.out.println("The number is smaller");
} else if (guess < randNumber) {
System.out.println("The number is higher");
}
}
if (numberOfTries >= maxAttemp) {
System.out.println("You reached the max Number of attempts :-/");
}
}
public static int nextValidInt(Scanner s) {
while (!s.hasNextInt() && s.hasNext()) {
System.out.println("Please provide a valid integer value!");
s.next();
}
return s.nextInt();
}
3 Answers 3
Not a full review, just a couple of points that jumped out at me:
- you often assign the value to a variable on the next line after declaring it. Just assign the value on the same line (eg
int startRange = nextValidInt(input);
). - you can declare a loop variable inside the loop:
for (int numberOfTries = 0; numberOfTries <= maxAttemp - 1; numberOfTries++)
. if (numberOfTries >= maxAttemp)
will always be true, as thefor
loop increasesnumberOfTries
tomaxAttemp
. Well, except when you won. I would remove theif
and replace thebreak
with areturn
.numberOfTries <= maxAttemp - 1
looks better written asnumberOfTries < maxAttemp
.- the
main
method shouldn't do anything except start your program. I would move your code to its own function. You might also want to create extra functions for initial info printing/setup gathering and the actual guessing game.
Not so random Random
bug
int randNumber = randGenerator.nextInt((endRange - startRange + 1)
+ startRange);
Let's say that startRange = 10
, and endRange = 25
, then this will evaluate to:
int randNumber = randGenerator.nextInt((25 - 10 + 1) + 10);
int randNumber = randGenerator.nextInt((16) + 10);
int randNumber = randGenerator.nextInt(26);
Which will produce numbers in the range 0 -- 25
. But... startRange
was set to 10
!?
That's because your parenthesis is incorrect. Correct would be:
int randNumber = randGenerator.nextInt(endRange - startRange + 1) + startRange;
Which evaluates, with the same example numbers, to:
int randNumber = randGenerator.nextInt(25 - 10 + 1) + 10;
int randNumber = randGenerator.nextInt(16) + 10;
So the lowest possible number is 0 + 10
and the highest 15 + 10
, that looks about right!
A variable name
int maxAttemp;
I'm not quite sure why you left that variable name unfini
I'd recommend naming it maxAttempts
. Cutting the last two characters helps no one.
If-else-if-else-if...else?
if (guess == randNumber) {
...
} else if (guess > randNumber) {
...
} else if (guess < randNumber) {
...
}
I can assure you, if a number is not equal to, and not more than another number, it will be less than.
if (guess == randNumber) {
...
} else if (guess > randNumber) {
...
} else {
...
}
Or, as I would prefer to write it: If it's not more, and not less, then it is equal.
if (guess > randNumber) {
...
} else if (guess < randNumber) {
...
} else {
...
}
By naming your function nextValidInt, you have limited yourself. The only input that you can ever get is an integer. If you wanted to get a string (e.g. "exit"), you'd have to refactor the function to name it something like nextValidToken. The code would need to change as well, since the return type would no longer work.
public static void findNextValidToken(Scanner s) {
while ( ( ! s.hasNextInt() && ! s.hasNext("exit") ) && s.hasNext()) {
System.out.println("Please provide a valid integer value or type exit to quit!");
s.next();
}
}
In your for loop, I don't see any reason for the inconsistent variable names. Why tries and attempts? Why not
for (int attemptCount = 0; attemptCount < attemptMaximum; attemptCount++) {
-
\$\begingroup\$ Could you give me a hint on how to do refactor the code, to make "exit" work? \$\endgroup\$chritschl– chritschl2014年10月14日 13:50:28 +00:00Commented Oct 14, 2014 at 13:50
GuessingGame
class that handles all the logic of the game (e.g. number of guesses remaining), while leaving all the input/output inmain
, then posting that for review. \$\endgroup\$