Guess number game

Question

I'm making a simple guess a number game. In game, user should input minimum and maximum value and program should generate random number between this numbers, later user should try and guess the number.

This is one of my first programs, so I'd like to check my coding techniques, what should I do differently etc.

I have getInterval() methode that I'm not sure I've done in best way, first I thought I should past two arguments (min and max) and change them in methode, but looks like I can do that like that in Java, so I've done it this way.

I'm going to make this game a bit more advanced later, but just want to check if this part is ok and if I should change some things before I move on.

import java.util.Random;
import java.util.Scanner;
public class Main {
 public static void main(String[] args) {
 int min=0;
 int max=0;
 min = getInterval("Min"); // Get minimum for guessing interval
 max = getInterval("Max"); // Get Maximum for guessing interval
 int randomNumber = getRandom(min, max); // generate random number between Min and Max
 playRandom(min, max, randomNumber); // play the game
 }
 public static int getInterval(String text){
 System.out.println("Please Input " + text + " number for guessing: ");
 Scanner inputMin = new Scanner(System.in);
 return Integer.parseInt(inputMin.nextLine());
 }
 public static int getRandom(int min, int max){
 Random rand = new Random();
 return rand.nextInt((max - min) + 1) + min;
 }
 public static void playRandom(int min, int max, int randomNumber){
 boolean guess = false;
 int numGuesses = 1;
 while (!guess){
 System.out.println("Enter your guess: ");
 Scanner inputGuess = new Scanner(System.in);
 int userGuess = Integer.parseInt(inputGuess.nextLine());
 if (userGuess < randomNumber){
 System.out.println("Too low!");
 numGuesses++;
 }
 else if(userGuess > randomNumber){
 System.out.println("Too high!");
 numGuesses++;
 }else{
 System.out.println("You win! You tried " + numGuesses + " time(s) total");
 guess = true;
 }
 }
 }
}

Answer 1

Use a class to manage state instead of passing it around.

Right now your program has 3 variables that represent the programs state. min, max and randomNumber.

The state is being passed around to wherever it's needed as function arguments:

public static int getRandom(int min, int max) {...}
getRandom(min, max);
public static void playRandom(int min, int max, int randomNumber) {...}
playRandom(min, max, randomNumber);

You could simplify these method calls and method signatures by using a class that holds this state as fields, and making these methods part of the class.

---

public class Game {
 private int min;
 private int max;
 private int randomNumber;
 ...
 public void getInterval() {...} // parameter no longer needed
 public void getRandom() {...}
 public void playRandom() {...}
}

Instead of returning the min and max from getInterval, you would assign the result to these fields:

public void getInterval() {
 Scanner inputMin = new Scanner(System.in);
 System.out.println("Please Input Min number for guessing: "); 
 // return Integer.parseInt(inputMin.nextLine());
 this.min = Integer.parseInt(input.nextLine()); // the "this." is optional here.
 System.out.println("Please Input Max number for guessing: ");
 this.max = Integer.parseInt(input.nextLine());
}

Then in getRandom, you would use the fields instead of method parameters:

// return rand.nextInt((max - min) + 1) + min;
// this.randomNumber = rand.nextInt((this.max - this.min) + 1) + this.min;
randomNumber = rand.nextInt((max - min) + 1) + min; // leaving out "this." here.

---

The result would be simpler method calls and signatures, since there is no more need for the arguments:

Game game = new Game();
public void getRandom() {...}
game.getRandom();
public void playRandom() {...}
game.playRandom();

The state that was previously passed around is now managed internally by the class.

Answer 2

Naming

Take a look at the names of your variables and functions. If I encounter a function getInterval(..), I expect to get the internal variable interval back as a result, or at least an interval. You return... one integer, which is neither of those. A possible better alternative is to call it promptForInteger, but even in that case, the function does not always do what you expect it to do. Similarly, you have a variable inputMin in that function that could potentially contain a Scanner which would read the value for the max variable.

Recreating Random and Scanner

You are recreating Random every time you call getRandom(..). Luckily, you just call it once. In general, you should only have one random number generator per application. Similarly, you are recreating a Scanner variable every time you call getInterval(..), while there does not seem to be a clear reason why you would do that.

Prompting for an integer

You use getInterval(..) to prompt for an integer for min and max, and inline-code to prompt for guesses. If the user enters something that is not an integer... your code errors out with an uncatched NumberFormatException. This is likely not the intended behaviour.

I would suggest replacing it with

public class Game {
 private Scanner input;
 public static int promptForInteger(String prompt) {
 System.out.println(prompt);
 while(!input.hasNextInt()) {
 input.next();
 }
 return input.nextInt();
 }
}

Random

It is good to realize that Random does return pseudo-random numbers, and that not all numbers are equally likely to be chosen as the random number. Your function getRandom(..) will also throw an IllegalArgumentException if max < min. You never test if max >= min anywhere in your code. With functions that should return random numbers, it is always wise to leave a comment what range you want to have returned. In your case you want an integer between min and max with both min and max included. This is important, because normally min is included, but max is excluded.

• Caridorc: "not all numbers are equally likely to be chosen as the random number" <- citation needed
• Sumurai8: nextInt() is implemented as bits % n; according to the documentation, which slightly favours lower numbers. The documentation also says "Instances of java.util.Random are not cryptographically secure".
• Caridorc: Ok I understand. For future readers I leave this as a reference: stackoverflow.com/questions/17830823/…
• Goran B: I'm implementing improvements that you guys suggested, now I'm using promptForInteger() as you suggested, just have one question, in my code, I was using Integer.parseIntto return string as integer (if I understand correctly) and you are not using that, but code it's still working. I know that we do check if input can be interpreted as integer, but don't we need to prase it to integer before storing to int variable? Thanks
• Sumurai8: This is what input.nextInt(); does. It reads a token, and interprets it as an integer. See the documentation for what other token types you can directly get from Scanner. While the next token can't be interpretted as an integer, we skip over the token with input.next();.

Answer 3

Loop simplification

boolean guess = false;
int numGuesses = 1;
while (!guess){
System.out.println("Enter your guess: ");
Scanner inputGuess = new Scanner(System.in);
int userGuess = Integer.parseInt(inputGuess.nextLine());
 if (userGuess < randomNumber){
 System.out.println("Too low!");
 numGuesses++;
 }
 else if(userGuess > randomNumber){
 System.out.println("Too high!");
 numGuesses++;
 }else{
 System.out.println("You win! You tried " + numGuesses + " time(s) total");
 guess = true;
 }
 }
}

You can avoid the Boolean flag guess and use a for loop to count the numGuesses

for (int numGuesses = 1; true; numGuesses++){
 System.out.println("Enter your guess: ");
 Scanner inputGuess = new Scanner(System.in);
 int userGuess = Integer.parseInt(inputGuess.nextLine());
 if (userGuess < randomNumber){
 System.out.println("Too low!");
 }
 else if(userGuess > randomNumber){
 System.out.println("Too high!");
 }else{
 System.out.println("You win! You tried " + numGuesses + " time(s) total");
 break;
 }
 }
}

Counting loops are usually done with for and the flag was not needed as you were breaking immediately anyway.