"Guess a number" game

Question

I began the journey to be a self-taught programmer about a month ago and my biggest fear about teaching myself is developing bad habits and not realizing it.

I'm looking for criticism on this "Guess a Number" game. It's my first real "project" that I've invested serious time into so I feel like this is my first opportunity to be critiqued.

package guessANumber;

import java.util.Random;

import javax.swing.JOptionPane;

/**
 * @author Mike Medina
 */

/**
 * The "Guess a Number" game
 * */
public class Guess {

static boolean playAgain;
static int max; // The objective will be between 1 and this number
static int objective; // Users are trying to guess this number
static int userGuess;

public Guess() {}

    /**
     * Runs the game while playAgain == true
     */ 
    public static void main(String[] args) {

        do {
        setMax();
        setObjective();
        userGuess();
        playAgain();
        } while (playAgain == true);

    }

    /**
     * Asks the user to set the max value they will guess between and validates it
     */
    public static void setMax() {

        boolean valid = false;

        // Asks user for "max"
        while(!valid) {
            try {
                max = Integer.parseInt(JOptionPane.showInputDialog("You will try to guess a number between 1 and \"max\". Set max: "));
                valid = true;
            }
            catch (NumberFormatException e) {}
        }

        // Ensures user input for max is greater than 1 and fits in an int
        while (max < 1 || max > Integer.MAX_VALUE) {
            valid = false;
            while (!valid) {
                try {
                    max = Integer.parseInt(JOptionPane.showInputDialog("Max must be between 1 and " + Integer.MAX_VALUE + ": "));
                    valid = true;
                }
                catch(NumberFormatException e) {}
            }
        }

    }

    /**
     * Sets the objective between 1 and "max"
     */
    public static void setObjective() {

        Random rand = new Random();

        if (max == 1)
            objective = 1;
        else
            objective = rand.nextInt(max - 1) + 1;

        // Prints objective for testing
        System.out.println(objective);

    }

    /**
     * Takes in the user's guess, validates it, and tells them when they win
     */
    public static void userGuess() {

        // Prompts user for guess and ensures it's an integer
        do {
            userGuess = 0;

            try {
                userGuess = Integer.parseInt(JOptionPane.showInputDialog("Guess a number between 1 and " + max + ": "));
            }
            catch (NumberFormatException e){}

            // Ensures user's guess is an integer greater than 0 and less than max
            while (userGuess > max || userGuess < 1) {
                try {
                    userGuess = Integer.parseInt(JOptionPane.showInputDialog(null, "Your guess must be between 1 and " + max));
                    }
                catch (NumberFormatException e) {}
            }
            userWinLose();
        } while (userGuess != objective);

    }

    /**
     * Tells the user whether their guess was high, low, or correct
     */
    public static void userWinLose() {

        // Tells the user their guess was too high, too low, or correct
        if (userGuess < objective)
            JOptionPane.showMessageDialog(null, "Too low!");
        else if (userGuess > objective)
            JOptionPane.showMessageDialog(null, "Too high!");
        else
            JOptionPane.showMessageDialog(null, "You win!");

    }

    /**
     * Asks the user if they want to play again or quit
     */
    public static void playAgain() {

        boolean valid = false;

        // Confirms user input is an integer
        while (!valid) {
            try {
                if (Integer.parseInt(JOptionPane.showInputDialog("Enter 0 to quit or 1 to play again: ")) == 0)
                    playAgain = false;
                else
                    playAgain = true;
                valid = true;
            }
            catch (NumberFormatException e) {}
        }

    }

}

Answer 1

On top of the comments already given :

Keep things simple :

• while (playAgain == true) can be written while (playAgain)
• if (A) variable=false; else variable=true; can be written variable = !A;
• Do not repeat yourself : if you deal a lot with user input and checking values, make it a function you can reuse.
• You do not need static members. Local variable can do the trick. The idea is to keep things in the smallest possible scope so that it's easier to understand what changes what.

Here's my code taking into account my comments :

import java.util.Random;
import javax.swing.JOptionPane;

/**
 * The "Guess a Number" game
 * */
public class Guess {
    public Guess() {}

    /**
     * Runs the game while playAgain == true
     */ 
    public static void main(String[] args) {
        do {
            int max = getMax();
            int obj = getObjective();
            userGuess(obj, max);
        } while (playAgain());
    }

    private static int getIntFromUser(String text, int min, int max)
    {
        while (true)
        {
            try
            {
                int val = Integer.parseInt(JOptionPane.showInputDialog(text));
                if (min <= val && val <= max)
                    return val;
            }
            catch (NumberFormatException e) { /* As pointed out by ChrisW, it might be worth having something here */}
        }
    }

    /**
     * Asks the user for the max value
     */
    public static void getMax() {
        return getIntFromUser("You will try to guess a number between 1 and \"max\". Set max: ", 1, Integer.MAX_VALUE);
    }

    /**
     * Gets the objective between 1 and "max"
     */
    public static int getObjective() {
        // Please read Simon André Forsberg's comment about this.
        Random rand = new Random();
        return (max == 1) ?
            1 :
            rand.nextInt(max - 1) + 1;
    }

    /**
     * Takes in the user's guess, validates it, and tells them when they win
     */
    public static void userGuess(int obj, int max) {
        int userGuess;
        do {
            userGuess = getIntFromUser("Guess a number between 1 and " + max + ": ", 1, max);
            userWinLose(userGuess, obj);
        } while (userGuess != obj);
    }

    /**
     * Tells the user whether their guess was high, low, or correct
     */
    public static void userWinLose(int guess, int objective) {
        if (guess < objective)
            JOptionPane.showMessageDialog(null, "Too low!");
        else if (guess > objective)
            JOptionPane.showMessageDialog(null, "Too high!");
        else
            JOptionPane.showMessageDialog(null, "You win!");
    }

    /**
     * Asks the user if they want to play again or quit
     */
    public static boolean playAgain() {
        return (getIntFromUser("Enter 0 to quit or 1 to play again", 0, 1) == 1);
    }
}

Answer 2

Overall it looks pretty good. A few pointers though...

• Try to indent your code properly. Most of it looks good but on a few places you have missed an indentation. e.g.,

  do {
  setMax();
  setObjective();
  userGuess();
  playAgain();
  } while (playAgain == true);
  

• Don't consume nor fail silently on exceptions (except in some rare cases but that's out of scope here)

  catch (NumberFormatException e) {} // don't do this
  

• Surround your statements with appropriate curly brackets

  if (max == 1)                   // no brackets!
      objective = 1;
  else                            // no brackets!
      objective = rand.nextInt(max - 1) + 1;
  

Some people don't do it out of habbit, or coding style, etc. It may not be a problem but as your codebase expands it could become a problem and IIRC Java style docs advice against omitting curly brackets. Here's an example of why it may be a bad idea https://stackoverflow.com/a/8020255/1021726

• max > Integer.MAX_VALUE Will never happen

Because max is declared as an integer and assigned the value entered in the JOptionPane. In the event of an user would enter a value which is higher than Integer.MAX_VALUE it would throw an exception. And since you do not do anything with your exceptions it will just fail silently. See point #2.

• static vs non-static

Do you understand static? If not, then you can get a quick brief here http://www.caveofprogramming.com/frontpage/articles/java/java-for-beginners-static-variables-what-are-they/

If you understand static, you may realize that you do not want two instances of your Guess class to share the same variable.

• Declare the access modifier

  static boolean playAgain; // no access modifier
  

While your variables lack access modifiers (and I do not think it's by purpose), and it may not be a problem as then it defaults as package-private, but you need to beware that it's important to design your class, it's members and variables accordingly to how they should be used. Are your variables really package-private, or should they rather be declared as just private variables?

Answer 3

Both Josay and Max have given great answers. There are two things I would like to add.

1. There is no need to carry the playagain boolean variable. You can simply:

   do {
       ....
   } while (playAgain());
   

2. There is a bug in your getObjective() method. You will never generate the 'max' value itself (unless it is 1). From the Java API Documentation for nextInt(n):

   Returns a pseudorandom, uniformly distributed int value between 0 (inclusive) and the specified value (exclusive)`

   ... so you ask the user to enter a max, lets say, for example, they enter 5. Now, your getObjectiveMethod does rand.nextInt(max - 1) + 1; which will generate a random number from 0 through (5-1), which, since (5 - 1) is 4, and the random value will be exclusive of 4, the largest random number generated will be 3. Add 1 to this again, and you get 4. Thus, your largest Objective will never be 5, or the max value.

This Random Number generation is a common problem to encounter. There are a number of posts and blogs about this, but the logical pointer to direct you to is this StackOverflow answer: Generating Random numbers in a range

The bottom line is that you want your number generated as:

rand.nextInt(max) + 1;

As a consequence of this, there is no need to have a special case for max == 1. You only need to ensure that max > 0 when it is entered (which you should do before setting valid = true

Answer 4

To throw you in at the deep end it would be good to start thinking more about an object-oriented design for your classes and interfaces. Think about re-usability of your code and how can encapsulate just enough useful behaviour in a class while keeping it as general as possible.

Let's say it becomes known that you've written a wonderful class to play this guessing game and other developers want to use it in their own code. Only one guy wants to use a different GUI, another wants to have a device make differently pitched beeps depending on whether the guess is too high or low and another wants to use the game with very young kids and always set max to 10. As it stands they can't use your code because implementation specifics appear at every level.

Let's focus on just the operation of the game. The interface is clear: a client must be able to communicate a guess to the class, and the class needs to provide an indication of whether the guess is too high, too low, or correct. So define IGuessHandler as a guess-handling interface comprising just that, and now have a class inherit it. You can then provide precisely the same implementations as now but someone else could implement different ones.

I think you should also extract your main into some GameRunner class and generally try to have more precise names for classes and operations. Your main loop has a playAgain method which is run when we play for the first time. It seems trivial but it's sloppy thinking. Don't have a method called playAgain, have one called play which actually plays the game. You call it again and again but keep the names as clear and descriptive as possible.

Answer 5

One thing you should know about exceptions is that you don't need to catch them immediately.

The reason why code throws an exception in the exception in the first place is that it doesn't know how to handle the exception.

Similarly, the one-higher subroutine (for example, your getIntFromUser method) which immediately called that code might also not know how to handle the exception.

It's fine, perhaps normal, to catch the exception in a different routine much higher up the call stack.

For example, here's a modified version of Josay's answer:

private static int getIntFromUser(String text, int min, int max)
{
    // parseInt might throw NumberFormatException
    int val = Integer.parseInt(JOptionPane.showInputDialog(text));
    if (min <= val && val <= max)
        return val;
    else
        throw new IllegalArgumentException("Not an integer between " + min + " and " max);
}

// display error message from caught exception
// return true if the user wants to play again
private static bool getIsContinueAfterError(String errorMessage)
{
    String message = errorMessage + "\n\nDo you want to continue playing?";
    int reply = JOptionPane.showConfirmDialog(null, message, "Error", JOptionPane.YES_NO_OPTION);
    return (reply == JOptionPane.YES_OPTION);
}

public static void main(String[] args) {
    boolean playAgain;
    do {
        try {
            int max = getMax();
            int obj = getObjective();
            userGuess(obj, max);
            playAgain = playAgain();
        }
        catch (NumberFormatException ex)
        {
            playAgain = getIsContinueAfterError("This is not an integer.");
        }
        catch (IllegalArgumentException ex)
        {
            playAgain = getIsContinueAfterError(ex.getMessage());
        }
    } while (playAgain);
}

This isn't necessarily a good example (in your example, you can catch and retry inside getIntFromUser); I meant it as an illustration because you said that you're not familiar with exceptions.

With your code, perhaps the user didn't enter an integer because all the number keys are broken on their keyboard; your code would catch and retry forever, whereas my code gives the user an opportunity to stop playing.

Answer 6

with the playagain boolean variable you should set it to false when you create the variable and then the loop can be a simple while(playagain) a lot of people prefer a while loop over a do while loop because you see what the condition is right out of the gate.

EDIT: now that I read through the Code again I see why you went with the do while and it completely makes sense, except for the following:

your PlayAgain method should return a boolean value. that being said it should be caught and passed to a variable inside your Main() method something like keepPlaying and that variable should be created inside your Main() method. the playAgain boolean value is only used in two places, Main() and playAgain(), which means that you shouldn't make it global, it is specific and should be specific in scope, it doesn't affect any of the other methods in the program.