Problem
I am a first year CS student. We are currently learning Java and my latest assignment was to create this random number game. I am hoping to get some feedback on code style etc so that I know what not to do in the future and so that I can avoid getting into bad habits from the start. I don’t know all that much so any explanations/tips at all will be amazing!
import java.util.Random;
import java.util.Scanner;
public class Main {
public static void main(String[] args) {
Scanner scan = new Scanner(System.in);
boolean win = false;
boolean running = true;
System.out.println("Please enter the upper limit: ");
Game game = new Game(scan.nextInt());
while (running) {
System.out.println("Please enter a guess ");
while (!win) {
if (game.guess(scan.nextInt())) {
win = true;
}
}
System.out.println("Would you like to play again y/n?");
String playAgain = scan.next();
if (playAgain.equalsIgnoreCase("y")) {
game.reset();
running = true;
win = false;
} else {
running = false;
System.out.println("Game over");
}
}
}
}
Game class:
import java.util.Random;
public class Game {
private int randomNumber;
private int tries = 0;
private int range;
public Game(int range) {
this.range = range;
generateRandomNumber();
}
private void generateRandomNumber() {
Random rand = new Random();
this.randomNumber = rand.nextInt(range) + 1;
}
public boolean guess(int guess) {
tries++;
if (guess == randomNumber) {
if (tries > 1) {
System.out.println("You got the number in " + tries + " tries.");
return true;
} else {
System.out.println("You got the number in " + tries + " try.");
return true;
}
} else if (guess > randomNumber) {
System.out.println("Too high try again");
return false;
} else if (guess < randomNumber) {
System.out.println("Too low try again ");
return false;
}
return false;
}
public void reset() {
this.tries = 0;
generateRandomNumber();
}
}
Solution
Unused imports – Warnings
You’re using Eclipse. Eclipse and NetBeans are IDE’s that will probably (aside from turning them off) give you warnings about your code.
An unused import is such a thing.
In Main, you don’t use java.util.Random. So you should remove it.
Your IDE will give you other warnings from time to time. If you look at the warnings it gives you, it will help in both debugging (“hey, you’re calling a function on a null variable here!”) and in cleaning up your code (“Use String.isEmpty() instead of string == ""” – I know NetBeans even just allows you to click a button and it will fix it for you).
Duplicate Code – Refactoring
Whenever you can, try to not state things multiple times.
Take this function.
public boolean guess(int guess) {
tries++;
if (guess == randomNumber) {
if (tries > 1) {
System.out.println("You got the number in " + tries + " tries.");
return true;
} else {
System.out.println("You got the number in " + tries + " try.");
return true;
}
} else if (guess > randomNumber) {
System.out.println("Too high try again");
return false;
} else if (guess < randomNumber) {
System.out.println("Too low try again ");
return false;
}
return false;
}
What it’s supposed to do:
Increase tries by 1. If the guess is correct, report the number of tries taken. If the guess is incorrect, report whether it is lower or higher than the hidden number. Return true if the guess was correct, and return false if the guess was incorrect.
Now right now you’re just comparing one int with another, so there’s no reason to define guess == randomNumber as isGuessCorrect yet.
What we can do, however, is remove duplicate code from branches.
if (tries > 1) {
System.out.println("You got the number in " + tries + " tries.");
return true;
} else {
System.out.println("You got the number in " + tries + " try.");
return true;
}
That code can become this:
if (tries > 1) {
System.out.println("You got the number in " + tries + " tries.");
} else {
System.out.println("You got the number in " + tries + " try.");
}
return true;
Another thing you could do, since it’s a simple string, is to use a ternary:
System.out.println("You got the number in " + tries + ((tries > 1) ? " tries." : " try."));
return true;
A similar thing can be done to the “too high too low part”.
But here’s another special case…
If a number is not equal to and not lower to a certain value, then obviously it must be higher than that value.
So the code
if(is equal){
print got number in x tries
return true
} else if(is lower){
print try higher
return false
} else if(is higher){
print try lower
return false
}
Could also be
if(is equal){
print got number in x tries
return true
} else if(is lower){
print try higher
return false
} else {
print try lower
return false
}
Which would make the final snippet look something like this:
public boolean guess(int guess) {
tries++;
if (guess == randomNumber) {
System.out.println("You got the number in " + tries + ((tries > 1) ? " tries." : " try."));
return true;
} //implicit else because return would end the function
if (guess > randomNumber) {
System.out.println("Too high try again");
} else {
System.out.println("Too low try again ");
}
return false;
}
You might even want to consider exporting this whole message printing to separate functions:
public boolean guess(int guess) {
tries++;
if (guess == randomNumber) {
printCorrectGuess(tries);
return true;
} //implicit else because return would end the function
if (guess > randomNumber) {
printTooHigh();
} else {
printTooLow();
}
return false;
}
private void printCorrectGuess(int tries){
System.out.println("You got the number in " + tries + ((tries > 1) ? " tries." : " try."));
}
private void printTooLow(){
System.out.println("Too low try again ");
}
private void printTooHigh(){
System.out.println("Too high try again");
}
And (we can always go further) extracting the condition can make it easier to handle return values:
public boolean guess(int guess) {
tries++;
boolean isGuessCorrect = guess == randomNumber;
if (isGuessCorrect) {
printCorrectGuess(tries);
} else if (guess > randomNumber) {
printTooHigh();
} else {
printTooLow();
}
return isGuessCorrect;
}
private void printCorrectGuess(int tries){
System.out.println("You got the number in " + tries + ((tries > 1) ? " tries." : " try."));
}
private void printTooLow(){
System.out.println("Too low try again ");
}
private void printTooHigh(){
System.out.println("Too high try again");
}
Which turns a large function into several smaller functions, each easier to grasp than the original function you started with.
This is a pretty good implementation of what you’ve set out to do. The naming of things is clear and the code is pretty well split up into methods.
Main Class
I’d maybe put a slightly clearer explanation of what the user is expected to do at the start, as well as your “enter an upper limit”. This is usually an excuse to break out your finest ascii art.
Something else to consider is what if your user doesn’t enter an integer? That will break your program, so putting in some error handling that asks your error-prone meatbag to try again and get it right this time, please. Your way of checking is to put the scan.nextInt() in a try/catch block and catch an InputMismatchException.
When you’re asking the player if they want to play again, you set running to true, but there’s no way for it not to be true, so that’s unnecessary.
Game class
You’re seeding rand every time you’re calling generateRandomNumber(), which isn’t really going to be an issue here, as you’re not calling that method quickly/often, but you should really only seed a random number generator once in a program, otherwise you’ll start having it return the same value over and over again, so maybe move your Random rand = new Random() to the constructor?
In guess(), I’d rearrange the first section to this:
if (guess == randomNumber) {
if (tries > 1) {
System.out.println("You got the number in " + tries + " tries.");
} else {
System.out.println("You got the number in " + tries + " try.");
}
return true;
}
You’re repeating yourself less and the return value is the same regardless. The same goes for the next part, instead, I’d have:
else if (guess > randomNumber) {
System.out.println("Too high try again");
} else if (guess < randomNumber) {
System.out.println("Too low try again ");
}
return false;
This looks quite good to me. In particular, I like how Game is its own class. That was the right decision, so good job.
One thing I would recommend is not having “side effects” in Game. In this case, that means not writing to System.out; move the output up to main, where you’re also doing the input. Though it doesn’t make much difference in a small program like this, it generally leads to more reusable, testable code, and is a good habit to get in to.
The only other thing I can see right now is that you could make range final. That means that once assigned to, its value cannot be changed. It makes your intention clear to people reading your code, and the compiler will tell if you that rule has been violated.
Your main() function is clumsy due to the superfluous use of two flag variables, win and running. In general, using flag variables to control execution flow is poor style. There are more assertive ways to get to where you want to go:
- To run the game at least once, then possibly again and again, use a do-while loop.
- Where practical, write the loop condition to perform the right test.
public static void main(String[] args) {
Scanner scan = new Scanner(System.in);
System.out.println("Please enter the upper limit: ");
Game game = new Game(scan.nextInt());
do {
game.reset();
System.out.println("Please enter a guess ");
while (!game.guess(scan.nextInt())) {
// Loop until a correct guess
}
System.out.println("Would you like to play again y/n?");
} while (scan.next().equalsIgnoreCase("y"));
System.out.println("Game over");
}
As for Game.guess(), I would restructure the conditions with an assertion to emphasize that exactly one of the three branches must apply. (Your final return false; should be unreachable.) The pluralization of “try” can be treated as an internationalization problem to be solved with MessageFormat. (Pluralization in English is simple relative to some other languages. MessageFormat is better prepared to handle such complexity. You might as well take advantage of the built-in library that is built to solve the pluralization problem.)
import static java.text.MessageFormat.format;
…
public boolean guess(int guess) {
tries++;
if (guess > randomNumber) {
System.out.println("Too high try again");
return false;
} else if (guess < randomNumber) {
System.out.println("Too low try again ");
return false;
}
assert guess == randomNumber;
System.out.println(format("You got the number in {0} {0,choice,1#try|1<tries}.", tries));
return true;
}
Nice start!
You should move all the game-control logic to Game, so that your main loop will be
while (!game.isGameOver())
{
game.nextMove()
}
You can code the ‘Do you want to play another?’ nicely outside this loop.
I would also introduce a Player, which can be asked for a move. So the Game will ask the Player for input:
class Game
{
public Game (Player p)
{
player = p;
}
Player player;
public void nextMove()
{
//or something similar
guess(player.getNextInput());
}
}
You can move all the IO (System.out etc) to the Player class, because Game does not need to know about it.
You now can easily implement a TestPlayer or a AIPlayer without having to change anything in the game logic.
As guess() is game logic, it should return something the Game knows, and notify the player. For example a MoveResult, which might contain some code (enum), variables and a message.
public MoveResult
{
public static enum {TOO_HIGH, TOO_LOW, RIGHT}
String message;
}
And in Game
public void nextMove()
{
//or something similar
MoveResult result = guess(player.getNextInput());
player.notify(result);
}