Beginner number guessing game

Question

I am a beginner to Java and have just written a number guessing game which involves guessing numbers from 1 to 10. I would like to improve my code by sticking strictly to conventions, improving code efficiency and forming good coding habits from the beginning.

Main class:

public class Main {

    public static void main(String[] args) {
        while (GuessingGame.play) {
            GuessingGame.play();
        }
    }
}

GuessingGame class:

import java.util.Random;
import java.util.Scanner;

public class GuessingGame {

    public static boolean play = true;

    private static int number = 0;
    private static int guess = 0;
    private static Random random = new Random();
    private static Scanner scanner = new Scanner(System.in);

    private static int nextInt(Random r, int lower, int higher) {
        int ran = r.nextInt(higher - lower);
        return (int) lower + ran;
    }


    public static void play() {
        number = nextInt(random, 1, 10);
        System.out.println("Insert your guess from 1-10!");
        guess = scanner.nextInt();
        if (guess == number) {
            System.out.println("Correct!");
            play = false;
        } else {
            System.out.println("Wrong! The number is: " + number);
            System.out.print("Do you want to try again? Y/N");
            String s = scanner.next();
            play= s.equalsIgnoreCase("Y") ? true : false;
        }
    }
}

Answer 1

There's no point in writing a separate class just to contain the main() function. I'd rather invoke java GuessingGame than java Main.

while (GuessingGame.play) is bad in several ways:

• GuessingGame exposes a public variable. It's standard practice to require Main to call a method, so that no other class can write to a variable in the GuessingGame class.
• The method GuessingGame.play() and the variable GuessingGame.play are too similarly named, which is confusing.
• GuessingGame.play() sets the GuessingGame.play flag as a side-effect, which is unconventional and unintuitive. It also violates the Single Responsibility Principle: play() should do one thing only.
• A do-while loop might be more appropriate, since you want to play at least one game.

In GuessingGame, everything is static. The number and guess variables could easily be made into local variables in play(), and therefore they should be.

Since the Random object is a class variable, there is no point in passing it as a parameter to nextInt(). There is a bug in nextInt(), in that it will never generate a 10.

Suggested solution

import java.util.Random;
import java.util.Scanner;

public class GuessingGame {

    private static Random random = new Random();
    private static Scanner scanner = new Scanner(System.in);

    /**
     * Picks a number uniformly between lowerBound (inclusive)
     * and upperBound (exclusive).
     */
    private static int nextInt(int lowerBound, int upperBound) {
        return lowerBound + random.nextInt(upperBound - lowerBound);
    }

    /**
     * Plays one game.
     *
     * @return true if the guess was correct
     */
    public static boolean play() {
        int number = nextInt(1, 10 + 1);
        System.out.println("Insert your guess from 1-10!");
        if (scanner.nextInt() == number) {
            System.out.println("Correct!");
            return true;
        } else {
            System.out.println("Wrong! The number is: " + number);
            return false;
        }
    }

    /**
     * Asks if the user wants to play again.
     */
    public static boolean shouldPlayAgain() {
        System.out.print("Do you want to try again? Y/N ");
        return "Y".equalsIgnoreCase(scanner.next());
    }

    /**
     * Plays guessing games until the user guesses correctly or gives up.
     */
    public static void main(String[] args) {
        while (!play() && shouldPlayAgain());
    }
}

Answer 2

• Why have you made these methods static?

  In this case if multiple users try to play the same game at the same time then variable number and guess shared by all thread, which is not desired because every played has its own number and own guess.

• In this code:

  play= s.equalsIgnoreCase("Y") ? true : false;
  

  equalsIgnoreCase is already returning true and false, so you need not check true and false and set the value based on the same.

  Your code should be:

  play= s.equalsIgnoreCase("Y");
  

• In this code:

  number = nextInt(random, 1, 10);
  System.out.println("Insert your guess from 1-10!"); 
  

  If you are assuming that the range may vary, then take the range as an input argument in the method and use the same in your printing message:

  public void play(int n, int m) { 
      number = nextInt(random, n, m); 
      System.out.println("Insert your guess from " + n + "-" + m + "!");