4
\$\begingroup\$

I've made mine first Java program and it was Guess a number game and got some great code review, so I've decided to first implement that suggestions and to make a game a bit more complex/advanced.

So now, the user can select between 3 options, 3 different intervals (1-10, 1-20, 1-100), but also, in each interval there are few mines he must avoid.

Eclipse suggested to set private int max; to bet private static int max; and some other private int's as well, wasn't sure why?

Let me know if something is not clear in explanation of game.

Here is the code:

Main.java

public class Main {

    public static void main(String[] args) {
        Game game = new Game();
        game.options();
        game.playGame();
    }
}

Game.java

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

public class Game {
    private static int min=1;
    private static int max;
    private static int numberOfMines;
    private int randomNumber;
    int mine[] = new int[20];
    private int numGuesses;
    private int userGuess;

    public static int promptForInteger() {
        Scanner input = new Scanner(System.in);
        while(!input.hasNextInt()) {
            System.out.println("Please enter valid number");
            input.next();
        }
        return input.nextInt();
    }    

    public void options(){
        int option = 0;
        do{
        System.out.println("Please select your game: ");
        System.out.println("1) Guess number between 1 and 10 with 1 mine.");
        System.out.println("2) Guess number between 1 and 20 with 3 mines.");
        System.out.println("3) Guess number between 1 and 100 with 20 mines.");
        option = promptForInteger();

        }while (option == 0 || option > 3);

        switch (option) { // set max depending on user selection
        case 1: max = 10;
                numberOfMines = 0; // one mine
                break;
        case 2: max = 20;
                numberOfMines = 2; // 3 mines
                break;
        case 3: max = 100;
                numberOfMines = 19; // 20 mines
                break;                
        }        
        createNumberAndMines();
    }

    public void createNumberAndMines(){
         randomNumber = getRandom(); // get random number to guess
         askForGuess(); // first user guess can't be mine, so I want to ask for first guess then set mines


         for (int i = numberOfMines; i >= 0; i--){

             do{ // we don't want to have same number for guessing and mine or first user guess to be a mine
                 mine[i] = getRandom();
                 for (int j = i+1; j<19; j++){ //check if generated number is duplicate mine
                     if (mine[i] == mine[j]){
                         i++; // if we find that this number is duplicated to previous one we want to do for loop again for this mine[i] 
                     }
                 }

             }while (mine[i] == randomNumber || mine[i] == userGuess);
         }
    }


    public static int getRandom() {
        Random rand = new Random();
        return rand.nextInt((max - min) + 1) + min; 
    }

    public void playGame() {
        for (int numGuesses = 1; true; numGuesses++){
            for (int i=numberOfMines; i>=0; i--){// check if user hits mine
                if (mine[i] == userGuess){
                    System.out.println("You lose! You have hit the MINE");
                    return;
                }
            }          

            // tell user how close he is to right answer
               if (userGuess < randomNumber){ 
                   System.out.println("Too low!");
               }else if(userGuess > randomNumber){
                   System.out.println("Too high!");
               }else if(userGuess == randomNumber){
                   System.out.println("You win! You tried " + numGuesses + " time(s) total");
                   break;
               }
               askForGuess();
        }
    }
    public void askForGuess(){
         System.out.println("You are guessing random number between 1 and " + this.max + ". Enter your guess: ");
         Scanner inputGuess = new Scanner(System.in);
         userGuess = Integer.parseInt(inputGuess.nextLine());
     }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ If you access members from static methods, the members have to be static as well (static methods aren't working against an instance of an object so can't access normal instance variables). Since you access min and max in getRandom which is static they need to be as well (or getRandom and any other static methods that reference them can be made non static). \$\endgroup\$ – forsvarir Jun 16 '16 at 10:03
7
\$\begingroup\$

Potential bugs

When asking the user to select their option for the game, negative values aren't checked:

do{
System.out.println("Please select your game: ");
System.out.println("1) Guess number between 1 and 10 with 1 mine.");
System.out.println("2) Guess number between 1 and 20 with 3 mines.");
System.out.println("3) Guess number between 1 and 100 with 20 mines.");
option = promptForInteger();

}while (option == 0 || option > 3);

If the user were to enter -7 for example, it would not match option == 0 || option > 3 so the while loop will exit. This will make the rest of the code crash because numberOfMines and max were not initialized.


Also, in the askForGuess() method, whose purpose is to ask the user for a guess, the code isn't checking that what the user entered is a valid integer. You already have a promptForInteger() method exactly for that! You can re-use it:

public void askForGuess(){
     System.out.println("You are guessing random number between 1 and " + this.max + ". Enter your guess: ");
     userGuess = promptForInteger();
 }

Reuse Random objects

To generate a random number, your current method is:

public static int getRandom() {
    Random rand = new Random();
    return rand.nextInt((max - min) + 1) + min; 
}

The issue with this is that, everytime the method is called, a new Random object is created. This produces mediocre quality random numbers. A Random object should be created just once, and be re-used. For example, you could make it a constant of the Game class.

Another possibility is to use the ThreadLocalRandom class, which makes it really easy to generate a random integer in a range:

public static int getRandom() {
    return ThreadLocalRandom.current().nextInt(min, max + 1);
}

(We need to have max + 1 since the upper bound is exclusive).

Use static only for global fields

A lot of the code is static:

private static int min=1;
private static int max;
private static int numberOfMines;

along with the methods getRandom() and promptForInteger(). Static fields are generally not a good idea. min, max and numberOfMines being static means that if we create two games, they will have the same value. This is probably not what you want: each game should have its own number of mines, etc.

As such, consider making them instance fields. You will also need to change getRandom() and make it an instance method since it operates on those instance fields.

promptForInteger() can also be made an instance method.

Unused code

private int numGuesses;

is unused in your code. Either remove it or use it, unused code shouldn't be left in code.

Hard-coded bounds, off-by-one checks

The maximum number of mines is hard-coded at multiple places of the code:

int mine[] = new int[20];
// ...
for (int j = i+1; j<19; j++)

This shows a small design issue: you are always creating a mine array of length 20 because that is the maximum the game can have. But when the game has less mines than 20, then the array is storing elements that will be of no use. Instead, this array should be instantiated with the correct number of elements, which is the number of mines in the game.

Additionally, numberOfMines, despite its name, does not represent the number of mines:

switch (option) { // set max depending on user selection
case 1: max = 10;
        numberOfMines = 0; // one mine
        break;
case 2: max = 20;
        numberOfMines = 2; // 3 mines
        break;
case 3: max = 100;
        numberOfMines = 19; // 20 mines
        break;
}        

This is awkward, why would numberOfMines be equal to 2 when there are 3 mines? Consider changing that so that numberOfMines accurately represents the number of mines in the game, instead of the number minus 1.


Your game has 3 options, each of them with a specific minimum value, maximum value and number of mines. Those values are hard-coded in the options printed to the user and in the switch.

You could create an enum of those option, which would look like

enum Options {
    EASY(1, 10, 1),
    MEDIUM(1, 20, 3),
    HARD(1, 100, 20);

    private final int min, max, numberOfMines;

    Options(int min, int max, int numberOfMines) {
        this.min = min;
        this.max = max;
        this.numberOfMines = numberOfMines;
    }
}

This would centralize all the options in one place. You can then loop over the options to print them and select the option chosen by the user (for example taking its ordinal as a first step).

Using the Stream API

The mine array is populated with random integers from min to max, with the exception from the first guess and the number to guess. Instead of having a while loop, you can do it easily using the Stream API:

mine = ThreadLocalRandom.current()
                        .ints(min - 1, max)
                        .filter(i -> i != userGuess && i != randomNumber)
                        .distinct()
                        .limit(numberOfMines)
                        .toArray();

This creates a stream of random integers between the bounds of the game, filters out the first guess and the number to find, only keeps numberOfMines of them and makes an array.

Checking if the user hit a mine is also easy to do:

if (Arrays.stream(mine).anyMatch(i -> i == userGuess)) {
    System.out.println("You lose! You have hit the MINE");
    return;
}

This checks if the array contains userGuess.


Lastly, in the following:

if (userGuess < randomNumber) {
    System.out.println("Too low!");
} else if (userGuess > randomNumber) {
    System.out.println("Too high!");
} else if (userGuess == randomNumber) {
    System.out.println("You win! You tried " + numGuesses + " time(s) total");
    break;
}

you don't need the else if (userGuess == randomNumber): if the number isn't strictly greater and strictly lower, then it must be equal. You can just have an else.

\$\endgroup\$
  • \$\begingroup\$ I'm looking a Stream API, it's something that I haven't use, but I can't see anywhere if it checks for duplicates, so that that game doesn't have duplicate numbers in mine array, is there any other way to implement it or should I use for loop? \$\endgroup\$ – Goran B Jun 16 '16 at 19:40
  • 1
    \$\begingroup\$ @GoranB I initially forgot to add it, but this is handled with the .distinct() operation. It will make sure that duplicates are not considered. \$\endgroup\$ – Tunaki Jun 16 '16 at 19:43
4
\$\begingroup\$

Static

I don't particularly like statics, it's very easy for them to be misused. Static members are shared across all instances of an object. In it's current form this isn't going to be a huge issue for your Game class, however it does restrict it's reuse. Since changes to max will impact all instances you can only really have a single Game running at a time. I'd make them into instance variables and amend the functions that reference them as necessary.

User Interaction

Consider pushing the user input/output away from your Game class. This isolates your Game logic from your user interaction. It'll make it much easier for you to replace the front end (for example with swing or a web interface) if you decide that you want to go in that direction. Something like (class/interface names aren't great but they convey what I'm trying to suggest):

public interface UserInteraction {
    int GetNumberFromUser(string prompt);
    void SendMessageToUser(string message);
}

public class ConsolerUserInteraction implements UserInteraction {
    int GetNumberFromUser(string prompt) {...}
    void SendMessageToUser(string message) {...}
}

Then you would supply the concrete implementation class when constructing your Game:

public static void main(String[] args) {
    ConsolerUserInteraction userInteraction = new ConsolerUserInteraction();
    Game game = new Game(userInteraction);
    game.options();
    game.playGame();
}

You could then have different implementations, for different types of user input/output.

numberOfMines

You're setting numberOfMines so that it's easier to use from your loop instantiations, however it's confusing.

numberOfMines = 0; // one mine

If the number of mines is 1, it should be 1, adjust the number when you need to for accessing the mine array.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.