Console-based number guessing game

Question

I received feedback that my code was convoluted and written with very bad practices.
Our tutor said explicitly that I should never write the code like that.

I'm just trying to wrap my head around the general concepts of object oriented programming.
I would be very thankful for pointing out what I did wrong, so I can better organize my programs in the future.

// Program.java
class Program {
 public static void main(String[] args) {
 Game numberGuessingGame = new Game();
 numberGuessingGame.play();
 }
}

// Game.java
import java.util.Scanner;
import java.util.concurrent.ThreadLocalRandom;
class Game {
 int numberRange;
 int numberToGuess;
 int tries;
 Scanner scan;
 Game() {
 initializeGame();
 selectRandomNumber();
 }
 private void initializeGame() {
 this.scan = new Scanner(System.in);
 }
 private void selectRandomNumber() {
 this.numberRange = getInt("Number range: ");
 resetGame();
 }
 private void resetGame() {
 this.numberToGuess = ThreadLocalRandom.current().nextInt(0, this.numberRange + 1);
 this.tries = 0;
 }
 int getInt(String message) {
 int number = 0;
 while (true) {
 System.out.print(message);
 try {
 number = this.scan.nextInt();
 }
 catch (java.util.InputMismatchException exc) {
 this.scan.nextLine();
 System.out.println("Incorrect input");
 continue;
 }
 break;
 }
 return number;
 }
 private int gameRound() {
 tries++;
 int guess = getInt("Make a guess (0 - " + this.numberRange + "): ");
 
 if(guess < numberToGuess) {
 System.out.println("Not enough");
 } if(guess > numberToGuess) {
 System.out.println("Too much");
 } if(guess == numberToGuess) {
 System.out.println("Nice! Number of tries: " + this.tries);
 System.out.print("Do you want to play again? [y/n] : ");
 
 // This line clears input buffer for scan
 this.scan.nextLine();
 while (true) {
 String answer = this.scan.nextLine();
 if(answer.matches("y|Y")) {
 resetGame();
 break;
 } else if(answer.matches("n|N")) {
 this.scan.close();
 return 1;
 } 
 System.out.print("[y/n] ? : ");
 continue;
 }
 }
 return 0;
 }
 public void play() {
 for(int i = 0; i == 0;) {
 i = gameRound();
 }
 }
}

• Toby Speight: Welcome to Code Review! We need to know what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task, rather than your concerns about the code.
• ggorlen: If your IDE has an autoformatter, it's not a bad idea to click that before submitting to your instructor or to CR. Did you instructor offer any specific points or did they just say the code was convoluted and written with bad practices without telling you which ones? That criticism seems a bit over-the-top to me -- while there are points for improvement, on the whole this code is reasonably clear and not something I'd be terribly unhappy to see from a first-year student.
• tanczmy: @ggorlen The main point was about the play() method being unclear. Other than that, he didn't mention anything specific, I feel it was more about the overall structure of the program. I just thought I was not seeing something obvious, the code seemed fine to me. And yes, you're absolutely right about the autoformatter, I need to set it up asap ;)
• ggorlen: OK, thanks for that. I suggest asking your instructor for more specifics. It's unhelpful to criticize something without explaining the rationale behind it, especially in an educational setting. Hopefully they can shed some light on what they were referring to.

Answer 1

A lot of your challenges stem from non-ideal state management - what should be static and what shouldn't, what should be a member and what shouldn't, and the lifetime of a class. Game should have final members so that it's immutable, and should be re-constructed every time that the user asks for a new game. resetGame, for similar reasons, is a code smell: rather than resetting an object, just drop its reference and allow it to be garbage-collected, constructing a new instance. In this case that's computationally cheap and preferable.

tries should be a local variable, and not captured in class state.

Your input loop should unconditionally nextLine, due to the semantics of Java scanners. This can be done with a finally.

Make better use of String.format() rather than string concatenation.

[y/n] would be better-written as (y|n), which (if anyone actually notices) is standard EBNF.

matches is overkill. You can lower-case the string and check that it begins with y or n.

Generally don't scan.close - it's typical for stdin to be left open. More importantly, ensure that there is a single scanner instance for stdin, which you already have - and will want to maintain for a modified class structure even when you iteratively construct instances.

i = gameRound(); doesn't make sense for a number of reasons. You're overwriting a loop variable and delegating responsibility for mutating it to a subroutine. You're also capturing a strange value, an integer representing continue-or-quit which should be a boolean. Instead, just loop over the number of tries as an integer.

initializeGame can go away and be part of the constructor.

Suggested

Program.java

import java.util.Scanner;
import static java.lang.System.out;
class Program {
 private final Scanner scan = new Scanner(System.in);
 public static void main(String[] args) {
 new Program().run();
 }
 public void run() {
 do {
 new Game(scan).play();
 } while (shouldPlayAgain());
 }
 private boolean shouldPlayAgain() {
 while (true) {
 out.print("Do you want to play again? (y|n): ");
 String answer = scan.nextLine().toLowerCase();
 if (answer.startsWith("y"))
 return true;
 if (answer.startsWith("n"))
 return false;
 }
 }
}

Game.java

import java.util.InputMismatchException;
import java.util.Scanner;
import java.util.concurrent.ThreadLocalRandom;
import static java.lang.System.out;
class Game {
 private final Scanner scan;
 private final int numberRange;
 private final int numberToGuess;
 public Game(Scanner scan, int numberRange) {
 this.scan = scan;
 this.numberRange = numberRange;
 numberToGuess = ThreadLocalRandom.current().nextInt(0, this.numberRange + 1);
 }
 public Game(Scanner scan) {
 this(scan, selectRandomNumber(scan));
 }
 private static int selectRandomNumber(Scanner scan) {
 return getInt(scan, "Number range: ");
 }
 private static int getInt(Scanner scan, String message) {
 while (true) {
 out.print(message);
 try {
 return scan.nextInt();
 }
 catch (InputMismatchException exc) {
 out.println("Incorrect input");
 }
 finally {
 scan.nextLine();
 }
 }
 }
 /**
 * @return true if won.
 */
 private boolean gameRound() {
 int guess = getInt(
 scan, String.format("Make a guess (0 - %d): ", numberRange)
 );
 if (guess < numberToGuess) {
 out.println("Not enough");
 return false;
 }
 if (guess > numberToGuess) {
 out.println("Too much");
 return false;
 }
 return true;
 }
 public void play() {
 int tries;
 for (tries = 1; !gameRound(); tries++) ;
 out.printf("Nice! Number of tries: %d%n", tries);
 }
}