Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

(Where this is my first post, I apologize in advance if this is formatted incorrectly and will gladly edit the post to reflect conventional formatting)

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) {}
        }

    }

}
share|improve this question
1  
the formatting can be part of the review. –  Malachi Jan 17 at 14:28
1  
I like the humility with which you are approaching programming. You'll learn faster than, and be easier to get along with, me when I started out. –  Wayne Conrad Jan 17 at 19:30

6 Answers 6

up vote 26 down vote accepted

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);
    }
}
share|improve this answer
4  
+1 but it worries me to see a silently-ignored exception. @MikeMedina Maybe use that to add an error message displayed to the user: String errorMessage; try { ....showInputDialog(errorMessage + text); ...; } catch (NumberFormatException e) { errorMessage = "That was not a number! Please try again.\n\n"; } or something like that. –  ChrisW Jan 17 at 13:04
    
Ok, I had a big mess of tangled while loops before I learned what try-catch was through some googling. I guess I started using it without fully understanding it but this makes sense! –  Mike Medina Jan 17 at 13:51
    
One though, that playAgain = playAgain() kind of irks me. Maybe just while(playAgain()), you are not using that variable elsewhere. Failing that maybe a rename? –  apieceoffruit Jan 17 at 15:25
    
@apieceoffruit You're right on this. The while (playAgain()); from rolfl'answer is much better. –  Josay Jan 17 at 15:26
1  
public static void getObjective() should return an int I believe :) And should also be written as return rand.nextInt(max) + 1; to generate from 1 to max inclusive. And Random objects are meant to be re-used! –  Simon André Forsberg Jan 17 at 16:42

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 http://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?

share|improve this answer
    
2. I was using try-catch in a loop to get the right input without showing an error. Is there a more graceful way to do this without abusing try-catch? 3. Hmm I thought it was good practice to avoid "excess" brackets if the if statement was one line but I understand how that could complicate things. Noted! 4. I thought if they entered a value higher than an int can hold, it would wrap around to negative. Also noted! 5. I guess this is part of OOP I don't understand yet. When would I ever use more than one instance of guess? 6. LoudandClear! –  Mike Medina Jan 17 at 12:03
1  
2. You could display a JOptionPane explaining that the user entered a number in the wrong range. This borders into the scope of good UI/UX, as now the user will now why his input failed. And what do you mean by abusing try/catch? 4. If you increment a value past Integer.MAX_VALUE then yes, but you cant assign an int a value higher than 2147483647 –  Max Jan 17 at 12:18
    
I guess I didn't understand the brackets of catch were for displaying an error message. Still a beginner! and 4. makes perfect sense. Thanks again! Do you have any example of a scenario when I would have more than one instance of guess? Like I said, I'm still sort of struggling with the implementation of the "object" part of object oriented programming ^^ –  Mike Medina Jan 17 at 14:37
1  
Well, they're not for displaying an error message, they are for handling an exceptional case that might occur when running the code. One of the many ways of handling exception cases includes logging the error message and displaying an error message to the user. 4. Imagine JRE is a casino and Guess is slot machines. If you would put a lot of slot machines in your casino and they all shared static variables, each slot machine would share the same state, same display, same output and have the same amount of money put into them. Now switch out slot machines into Guess instances and there you go. –  Max Jan 17 at 15:36
    
So if I need to access a variable in multiple different methods all in the same class, do I set the variable as private static? Or is there a way to avoid using static and still access one variable throughout the class? –  Mike Medina Jan 17 at 15:44

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

share|improve this answer
    
Thanks! I could have sworn I tested max at 3 a bunch of times to make sure I could get 3 etc. but I guess not. I read the documentation for rand but must have missed the upper bound as exclusive. Thanks for pointing this out! –  Mike Medina Jan 17 at 13:54

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.

share|improve this answer
1  
This is answer is EXACTLY what I was hoping to get when I decided to post here. These OOP concepts are out of my grasp because I can't understand how to use the OOP toolset to my benefit. Thank you for giving me examples on how I should be structuring my code from the start! If you have a chance, is it possible you could explain interfaces more in depth? I've actually spent A LOT of time researching interfaces and I just can't wrap my head around the concept. It seems to me like it they're just lists of methods that "should" be included. What good is a list of method titles? –  Mike Medina Jan 17 at 17:09
1  
However YAGNI is worth practising too. –  ChrisW Jan 17 at 17:09
1  
Just think of an interface as specifying a set of behaviour that any class implementing the interface must provide. Thinking about my previous answer I'd now change it a bit. There's no one right way to do this, but let's define IGuessHandler as an interface which has a handleGuess(int) method which returns an instance of a GuessOutcome base class. This can be characterised as having an internal state of TOO_LOW, TOO_HIGH, CORRECT (defined as enums) and a Render() method. You can then derive your own class from this with the same implementation as before. –  TheMathemagician Jan 17 at 17:25
1  
@MikeMedina It's tempting to over-complicate a design. Instead I'd suggest keeping it as simple as possible (without interfaces) until you need them. If you need them in future then you can get them by refactoring. The simpler it is now, the sooner it's written and the easier it is to refactor later, if necessary, after you know whether you want to add new functionality, and know what that functionality is. –  ChrisW Jan 17 at 17:41
1  
@MikeMedina IMO an 'interface' separate from a class is worthwhile when you want to 'plug-in' two different classes (two different implementations, or even just two different versions of the same class). E.g. in future you might want two types of guesser: 1) users who guess interactively 2) software that guesses automatically. Then, it might be worth defining interface IGuess { int getGuess(); } which is implemented by class UserGuess implements IGuess { public int getGuess() { return Integer.parseInt(JOptionPane.showInputDialog("What is your guess?")); } }. Until then, I wouldn't bother. –  ChrisW Jan 17 at 20:33

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.

share|improve this answer
1  
(I'm inexperienced with Java and JOptionPane so there may be syntax errors in the code above.) –  ChrisW Jan 17 at 14:53
1  
Thanks for your input! I thought I was finished with the features of the program before I posted it for review but now I realize that the "cancel" button in the JOptionPanes doesn't work because the loop just restarts. Definitely an issue not allowing users to exit the program safely! –  Mike Medina Jan 17 at 15:26

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.

share|improve this answer

protected by Malachi Mar 11 at 4:05

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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