Guess how many are in the jar game in Java

Question

I'm reading up on java and have completed a fairly simple console game.

I'm unsure on my own structure when considering maintainability, separation of concerns and best practices.

Any tips or suggestions is appreciated.

import java.io.*;
import java.util.Scanner;
import java.util.*;
class Prompter {
 private static Scanner scanner = new Scanner(System.in);
 public Prompter() {}
 public static String getInput(String title) {
 System.out.print(title);
 String input = scanner.nextLine();
 return input;
 }
 public static void getReadyforGame() {
 String line;
 do {
 System.out.print("Ready? (press ENTER to start guessing): ");
 } while ((line = scanner.nextLine( )).length( ) > 0);
 }
 public static void getGuess(Jar jar) {
 System.out.print("\nGuess: ");
 int answer = scanner.nextInt();
 while (answer != jar.getNumberToGuess()) {
 jar.incrementNumberOfGuesses();
 System.out.print("Guess: ");
 answer = scanner.nextInt();
 } 
 }
 public static void printResult(Jar jar) {
 System.out.println("\nCongratulations - you guessed that there are " + 
 jar.getNumberToGuess() + " lentils in the jar! It took you " + 
 jar.getNumberOfGuesses() + " guess(es) to get it right.\n");
 }
}
class Jar {
 private String itemName;
 private int numberOfItems;
 private int numberToGuess;
 private int numberOfGuesses;
 public Jar(String itemName, int numberOfItems) {
 this.itemName = itemName;
 this.numberOfItems = numberOfItems;
 this.numberToGuess = new Random().nextInt(numberOfItems) + 1;
 this.numberOfGuesses = 1;
 }
 public String getItemName() {
 return itemName;
 }
 public int getNumberOfItems() {
 return numberOfItems;
 }
 public int getNumberToGuess() {
 return numberToGuess;
 }
 public int getNumberOfGuesses() {
 return numberOfGuesses;
 }
 public void incrementNumberOfGuesses() {
 ++numberOfGuesses;
 }
}
class Admin {
 private Prompter prompter;
 public Admin () {}
 public Jar setupGame() {
 System.out.print("ADMINISTRATOR SETUP\n" +
 "===========================\n\n");
 boolean success = false;
 int numberOfItems = 0;
 String itemName = prompter.getInput("Name of items in the jar: ");
 while (!success) {
 try {
 numberOfItems = Integer.parseInt(prompter.getInput("Maximum of lentils in the jar: "));
 success = true;
 } catch (Exception e) {
 System.out.println("Number of items was invalid");
 }
 }
 return new Jar(itemName, numberOfItems);
 }
}
class Player {
 private Prompter prompter;
 public Player() {}
 public void playGame(Jar jar) {
 System.out.print("\n\nPLAYER\n" +
 "===========================\n" + 
 "Your goal is to guess how many lentils are in the jar. Your guess should be between 1 and " + jar.getNumberOfItems() + "\n\n");
 prompter.getReadyforGame();
 prompter.getGuess(jar);
 prompter.printResult(jar);
 }
}
class Game {
 private Admin admin;
 private Player player;
 private Prompter prompter;
 private Jar jar;
 public Game() {}
 public void start() {
 admin = new Admin();
 player = new Player();
 jar = admin.setupGame();
 player.playGame(jar);
 }
}
class Test {
 public static void main(String[] args) {
 Game game = new Game();
 game.start();
 }
}

Answer 1

Javadoc, Javadoc, Javadoc

---

import java.io.*;

Wildcard imports are sub-optimal. You never know what exactly is imported and it might conflict with other imports.

---

Classes should all be declared in their own files, this smells like you have all in one file.

---

class Test {

The main class should ideally be public. I also prefer to have Main somewhere in the name (or as name).

---

class Game {
 ....
 private Prompter prompter;

It is declared but never used.

---

You always create a new instance of Prompter but it only contains static methods. So either you make it a static helper class (and always invoke the methods statically) or you make it an instance class. I'd opt for an instance class.

---

String line;
do {
 System.out.print("Ready? (press ENTER to start guessing): ");
} while ((line = scanner.nextLine( )).length( ) > 0);

Why you need a line variable at all here?

---

Your logic is split over all the classes, each class has a little piece of the puzzle and the flow is just as far stretched. It is very hard to follow the actual program flow because you have to piece it together from all over the place.

Ideally, you'd have the main class, a class containing the actual logic and some helper classes. The main class would kick off your logic class, which would contain some setup code and the actual logic for the game logic.

Consider this pseudo-code:

class Main
 static void main()
 new Game().start()
class Game
 void start()
 setupPlayers()
 setupJar()
 do
 echo "Guess! "
 while getAnswer() != jarCount

Something like this. Abstract the details away behind methods but keep the main logic in one place, so that it is easy to follow the program flow.

Your main method should tell me what your application does, not how it does it.

On a related note, Your classes are encapsulating state that does not need to be in its own class. Jar is only used once, there speaks nothing against having these four variables in your Game class. Same for the Player class, which only contains a method which would be better in the Game class.

You have filleted the code into too many classes, all this could be done in one well organized class.

---

I have to say, with a few exceptions, your naming is always spot on and awesome! Good job.

• Nilzone-: Thanks for your input! I'm working on my new version now.
• Nilzone-: I'd really appreciate it if you have some time to look over my new version; codereview.stackexchange.com/questions/134357/…