Guess the random number game

Question

I have created the classic "guess the random number" game:

Generator Interface

 package ishan.guessingGame.randomNumberGenerators;

 public interface RandomNumber {

 public int generate();
}

Generator Implementation

package ishan.guessingGame.randomNumberGenerators;

import java.util.Random;

public class SimpleRandomNumberGenerator implements RandomNumber {

private int limit=50;

public SimpleRandomNumberGenerator(){

}
public SimpleRandomNumberGenerator(int limit){
    this.limit=limit;
}
@Override
public int generate() {

    return generateRandomNumber();
}

private int generateRandomNumber(){
    Random random=new Random();
    return random.nextInt(limit+1);
}
}

Game interface

package ishan.guessingGame.game;        

public interface Game {

public void startGame();
}

A Game Impl

package ishan.guessingGame.game;

import java.util.Scanner;

import ishan.guessingGame.randomNumberGenerators.RandomNumber;
import ishan.guessingGame.randomNumberGenerators.SimpleRandomNumberGenerator;

public class SimpleGuessingGame implements Game{

private int randomNumber;
private int difficultyLevel;
private Scanner scanner;
private RandomNumber random;
private int input=999;

public SimpleGuessingGame(){
    scanner = new Scanner(System.in);
}

@Override
public void startGame() {
    init();
    play();
    endGame();
}

private void init(){

    System.out.println("Enter The Difficulty : 1 = Easy , 2 = Moderate , 3 =Difficult");

    String difficulty;
    difficulty=scanner.next();

    if(!checkInputIsNumber(difficulty, true)){
        endGame();
    }

    difficultyLevel=Integer.parseInt(difficulty);

    if(difficultyLevel == 1){
        random=new SimpleRandomNumberGenerator();
    }
    else if (difficultyLevel == 2){
        random=new SimpleRandomNumberGenerator(100);
    }
    else{
        random=new SimpleRandomNumberGenerator(200);
    }
    randomNumber=random.generate();
}

private void play(){
    System.out.println("Input any character to exit..");

    while(true){
        System.out.println(" Guess The Number");
        String tempInput=scanner.next();

        if(!checkInputIsNumber(tempInput , false)){
            endGame();
        }

        input=Integer.parseInt(tempInput);
        if(input==randomNumber){
            System.out.println("You won.Bye!!");
            endGame();
        }
        else if(input > randomNumber){
            System.out.println("Easy there. Guess a smaller Number");
        }
        else{
            System.out.println("Pick a larger one");
        }
    }
}


private boolean checkInputIsNumber(String input , boolean checkForDifficulty) {

    if(checkForDifficulty){
        if(input.matches("^[0-3]")){
            return true;
        }
    }

    else if (input.matches("^[0-9]*")){
        return true;
    }
    return false;
}

private void endGame() {
    scanner.close();
    System.exit(0);
}
}

Main class

package ishan.apps;

import ishan.guessingGame.game.Game;
import ishan.guessingGame.game.SimpleGuessingGame;

public class MainApplication {

public static void main(String a[]){
    Game game=new SimpleGuessingGame();
    game.startGame();
}
}

I have tried to code to interfaces and would like to get suggestions around that area and my code in general.

Answer 1

Looking at the class SimpleRandomNumberGenerator :

1. Having generate() delegate to a private method is, in this case a bit overkill.
2. You create a new Random instance for each method invocation. This makes the SimpleRandomNumberGenerator not genrating an even distirbution of random numbers. Make it a private final field initialized at the declaration or in the constructor.
3. Good use of coding to interfaces, by hiding the implementation details behind an interface.

Then MainApplication :

1. Spot on : kept simple this class' responsibility is to wire everything together and kick things off.

And now for the meat SimpleGuessingGame :

1. Several fields of this class can be truned into local variables in the one method that uses them : difficultyLevel, random and input.
2. startGame() is well implemented, easy to read.

3. On the whole the class has at least 2 responsibilities :

   • handling user interaction
   • keeping track of game state

   That is too much. Imagine here you'd want to be able to play the same game but with a graphical UI.

   So we want to split this into more classes using interfaces to hide implementation from one another.

   I would suggest using these two interfaces :

   interface UserInteraction {
   
       String askDifficulty();
   
       void close();
   
       String askToGuess();
   
       void showHowToEndMessage();
   
       void showWinMessage();
   
       void showGuessSmallerMessage();
   
       void showGuessLargerMessage();
   }
   
   
   interface GameState {
   
       Result guess(int guess);
   }
   
   public enum Result {
   
       CORRECT, GUESS_LARGER, GUESS_SMALLER
   }
   

4. checkInputIsNumber() should be split into two separate methods : one for the difficulty and one for the guesses. They can then be named more aptly to increase readability (boolean parameters say so little at the call site)

5. the loop in the play() method is implemented as an infinite loop, while it is obviously driven by the game state. Meanwhile endGame() quite harshly exits, this makes it harder to embed the game in a larger program. Only the MainApplication class should call System.exit()

There's more to do as a follow up on these remarks, but I'd start there.

Answer 2

Style

• your indentation is off
• use more spaces (around = and +, before {, etc)

Just paste your code into any IDE, it can fix this easily for you, and it will make your code more readable.

Wrapper Methods

Whenever you have a method that doesn't do anything, but just calls another method, you can remove it.

Just copy the code of generateRandomNumber into generate.

checkInputIsNumber if statement

The if statements are a bit confusing. I would write it as:

    if (checkForDifficulty && input.matches("^[0-3]")) {
            return true;
    } else if (input.matches("^[0-9]*")) {
        return true;
    }
    return false;

Or even just:

return (checkForDifficulty && input.matches("^[0-3]")) || input.matches("^[0-9]*");

But I would actually make more methods out of this, because it seems like two different activities (check if it can be parsed as integer, and check if the integer is below 3). I would create a isInt method (or just use a try block), and then a isBelow(number, numberToCheck) method (or not. if you separate the functionality, it becomes easier to see that the functionality actually doesn't need to be extracted to a method).

Declare Variables in smallest scope

You should always declare variables in the smallest scope possible, that way a reader only has to have them in mind when they actually matter.

difficultyLevel for example is defined at class level, but only used in init.

Misc

• you can make random a field in SimpleRandomNumberGenerator instead of creating a new object each time.
• just exiting on malformed input isn't very user friendly.
• using 50 as the default value for a random number seems quite arbitrary.

Answer 3

Naming

Note that package names in Java should always have lower case. So ishan.guessingGame.randomNumberGenerators should be ishan.guessinggame.randomNumbergenerators.

Overrides

Why are you just returning another method here ?

@Override public int generate() {

    return generateRandomNumber(); 
}

generaeRandomNumber() is not complex and from what I can understand of your code, is not called anywhere else, so you could just implement it directly in generate.

You did great with startGame(), with making 3 call to private methods. By using private method you encapsulate what you're doing and that is great.

Random

I'm no expert with the Random class. There is a lot of subtleties with generating random numbers. What you need to be aware is that generating a new Random for generating a new number can screw the randomness. You should declare a private Random instance and use it in your generate method.

System.exit

I would strongly advise you to stop using this method. It's drastic! When you will deal with more complex application, you will have resources that you will want to close gracefully ( a database, a GUI and etc). System.exit() does not leave place to cleanly stop the application.

Let the application finish normally! It's not the responsibility of the Game to determine if the application should stop!

Answer 4

To add something that I believe the other answers missed, and is pretty important:

Inside SimpleRandomNumberGenerator you should, perhaps, impose some preconditions on the limit that gets passed. You might or might have not considered this, but public int generate() in your case throws an IllegalArgumentException (as the documentation also states) if the limit received in the constructor + 1 is not a positive integer. Perhaps you should let the clients know about it?