Number Guessing Game in Java

Question

This is my implementation of the famous Number Guessing Game. It is not object oriented, but the goal was to create a simple to read, procedural program. Have I accomplished this goal?

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

class Main {
  private static int lowerRange = 1;
  private static int upperRange = 99;
  private static int tries = 6;

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

  public static void main(String[] args) {
    showDescription();
    boolean run = true;
    while (run) {
      play();
      System.out.print("Play again? (Y / N): ");
      run = input.nextLine().equalsIgnoreCase("Y") ? true : false;
    }
  }

  private static void showDescription() {
    System.out.println("You have to guess a number between " + lowerRange + " and " + upperRange + ".");
    System.out.println("You have " + tries + " tries.");
  }

  private static void play() {
    int randomNumber = generateRandomNumber(lowerRange, upperRange);
    int triesSoFar = 0;
    boolean won = false;
    while (triesSoFar < tries && won == false) {
      System.out.print("Your try: ");
      int guess = Integer.parseInt(input.nextLine());
      if (guess > randomNumber) {
        System.out.println("The secret number is smaller.");
      } else if (guess < randomNumber) {
        System.out.println("The secret number is higher.");
      } else {
        won = true;
        break;
      }
      triesSoFar++;
    }

    if (won) {
      System.out.println("You found the secret number!");
    } else {
      System.out.println("You lose. The secret number was " + randomNumber + ".");
    }
  }

  private static int generateRandomNumber(int lowerRange, int upperRange) {
    return random.nextInt(upperRange - lowerRange) + lowerRange;
  }
}

Answer 1

You have showDescription directly printing out the message. Avoid printing in arbitrary functions whenever possible. It's much better to have things return Strings, then print at the call site as needed. I'd change that function to:

private static String produceDescription() {
    return "You have to guess a number between " + lowerRange + " and " + upperRange + ".\n"
           + "You have " + tries + " tries.");
}

. . .

System.out.println(produceDescription());

Why? Two highly-related reasons:

• Whenever possible, functions should return the data that they produce. Can you guarantee that for a function like this you will always want to directly print that data? Let the caller decide how they want to use the data that the function produces. Forcing the data to be printed makes the function less useful in the long term. As an example...

• If you ever decide to adapt this program to use a full GUI, you're directly printing and will need to modify every function that is calling println. The less functions you have that are using data in a specific way, the easier it will be to alter your program later.

---

I would not make everything static here. Again, what if in the future you wanted to run two games at the same time (like if you created a server that allows people to connect to it and play)? I'd get rid of static everywhere, and make everything plain instance methods/properties, then just instantiate a instance of the game in main.

---

Main is a poor name for this class. Ideally, it should be a description of what the object accomplishes. What if you ever imported this class so it can be used elsewhere? A class called Main doesn't make it immediately obvious what it is used for. I'd change the name to something like NumberGuessingGame.

---

run = input.nextLine().equalsIgnoreCase("Y") ? true : false;

is redundant. Ternary expressions are cool, but they're often overused. Think about it, what does equalsIgnoreCase return? A bool (true or false). You're then using that bool as a condition to the ternary... to get the same thing that equalsIgnoreCase returned originally.

Just get rid of the ternary:

run = input.nextLine().equalsIgnoreCase("Y");

And on the topic of conditions, a little later you have:

while (triesSoFar < tries && won == false) {

won == false certainly isn't wrong, but comparing against a boolean value directly is almost always unnecessary. Just write:

while (triesSoFar < tries && !won) {

Remember, ! is read as "not". "While tries to far is less than tries, and they haven't (not) won".

---

tries is a bad name. It isn't representing the number of tries taken (that's triesSoFar), it's representing the max number of tries allowed. Change it to something like maxAllowedTries.

Naming is very important. It is one of the key things that allows someone to be able to read your code and quickly know what's going on. Make sure your functions and variable names accurately describe what they do, or you'll make other people's lives more difficult when they need to read your code. You'll also make your own life more difficult if you ever come back to this program, because you will eventually forget parts of this program after some time.

---

System.out.print("Your try: ");

Isn't actually guaranteed to print right away. print and println use a "buffer" to hold text while waiting to print. That buffer is only "flushed" (printed out) when the text exceeds a certain length, or a newline (\n) is reached. You have short text here, and because you're using print, no newline is being added to the buffer. You may find that if you made this text a little shorter, it wouldn't print until some other text had be printed as well, which will make your program confusing.

If you use print instead of println, it can be a good idea to add a call to System.out.flush() after it to make sure everything is printed when you want it to be.

---

Just to show a potentially appropriate use of a ternary, you have:

if (won) {
  System.out.println("You found the secret number!");
} else {
  System.out.println("You lose. The secret number was " + randomNumber + ".");
}

Which isn't bad, but it could be shorted a bit:

String message = won ? "You found the secret number!"
                     : ("You lose. The secret number was " + randomNumber + ".");

System.out.println(message);

I'm not necessarily advocating for this way, but I thought I'd show it. It allows you to get rid of the multiple calls to println.

---

  int guess = Integer.parseInt(input.nextLine());
  if (guess > randomNumber) {
    System.out.println("The secret number is smaller.");
  } else if (guess < randomNumber) {
    System.out.println("The secret number is higher.");
  } else {
    won = true;
    break;
  }

is a dense chunk of code. I prefer to add more spacing around things. I like blank lines above ifs and elses so it's easier to see at a glance the distinct blocks. I'd make it:

  int guess = Integer.parseInt(input.nextLine());

  if (guess > randomNumber) {
    System.out.println("The secret number is smaller.");

  } else if (guess < randomNumber) {
    System.out.println("The secret number is higher.");

  } else {
    won = true;
    break;
  }

That generally makes it easier to pick things out by eye when scanning over a document.

---

---

There's some more stuff, but unfortunately, I have to go. Good luck!

Answer 2

I assume you have already ran it and can confirm it works. Seems to be fine. Only small thing is that you have to choose a number between 1 and 99, but although 99 cannot be the answer, 1 can be.

https://docs.oracle.com/javase/8/docs/api/java/util/Random.html

Answer 3

The static variables declared in the beginning of the class should be constants and named according to naming conventions.

private static final int LOWER_RANGE = 1;
private static final int UPPER_RANGE = 99;
private static final int MAX_TRIES = 6;

Things that are not meant to change should be final. It's debatable if these should be named like constants like above, as they are static and final, but in this context of pure procedural programming, the static keyord doesn't make much difference, so going lower case can be justified.

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

A number guessing game should handle invalid input too, so the code should catch NumberFormatException. However, now that the input reading became more complex, I would refactor it into a separate method. How errors and user's desire to stop playing is reported is again debatable, but since the context is procedural programming, I'm sticking to C-style magic return values.

/**
 * Read input from user.
 *
 * @return The user's guess or -1 if the user wants to stop playing.
 */    
private static int readGuess() { 
  ...
}