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.

Would you please review and give me some feedback? I've been learning Java and I want to avoid bad practices.

Is it good to make a helper class to define the methods my main program is using?

Is this approach ok for bigger projects?

If the game looks fine, what would be the next simple game challenge I should attempt? I'm not confident enough yet to mess with graphics.

I'm new to GitHub, but I created my first repository today to upload this code, perhaps its easier to look at the code here.

This is the main class:

package game;

import java.util.Scanner;

import utils.GuessNumberUtils;

public class GuessGame {

    // Class with all the methods required by the game
    static GuessNumberUtils numberGenerator = new GuessNumberUtils();

    // Scanner to handle input
    static Scanner keyboard = new Scanner(System.in);

    // This is where we store the number to guess
    static int numberToGuess;

    // This is where we store the number guessed by the player
    static int guessedNumber;

    // Number of tries - the idea is to handle a scoring system in the future
    static int tries;

    // Flag used to loop the game
    static boolean gameEnded = false;

    // This is where we store the player's name
    static String playerName;

    public static void main(String[] args) {

        // Asks for player's name
        playerName = GuessNumberUtils.getPlayerName(keyboard);

        while (!gameEnded) {
            // We initialize the score (tries)
            tries = 1;

            // Generates a random number. To do: ask the players for the range
            numberToGuess = numberGenerator.getRandomNumber(1, 100);

            // Debugging only. Remove this line to actually play the game.
            System.out.println(numberToGuess);

            System.out.println(playerName
                    + ", I've picked a number between 1 and 100, guess it!");

            guessedNumber = GuessNumberUtils.guessNumber(keyboard);

            while (guessedNumber != numberToGuess) {
                GuessNumberUtils.isNumberCorrect(numberToGuess, guessedNumber);
                tries++;
                guessedNumber = GuessNumberUtils.guessNumber(keyboard);
            }

            if (guessedNumber == numberToGuess) {
                if (tries == 1) {
                    System.out.println("Good job. You guessed it in 1 try.");
                } else {
                    System.out.println("Good job. You guessed it in " + tries
                            + " tries.");
                }
            }

            gameEnded = GuessNumberUtils.playAgain(keyboard);
        }
        keyboard.close();
    }
}

This is the helper class I used to define methods:

package utils;

import java.util.Scanner;

public class GuessNumberUtils {

    /*
     * Method to generate a random number in a specific range
     */
    public int getRandomNumber(int min, int max) {
        return min + (int) (Math.random() * ((max - min) + 1));
    }

    /*
     * Method to ask for the player's name
     */
    static public String getPlayerName(Scanner keyboard) {
        String playerName;
        System.out.println("Hi there! What's your name?");
        playerName = keyboard.nextLine();
        System.out.println("Hi " + playerName + ", let's play!");
        return playerName;
    }

    /*
     * Method to ask for a number to guess.
     */
    static public int guessNumber(Scanner keyboard) {
        System.out.println("Pick a number: ");
        return Integer.parseInt(keyboard.nextLine());
    }

    /*
     * Method to check if the player has guessed the number.
     */
    static public boolean isNumberCorrect(int numberToGuess, int guessedNumber) {
        if (numberToGuess == guessedNumber) {
            return true;
        } else {
            if (numberToGuess < guessedNumber) {
                System.out.println("Too high. Guess again.");
            } else {
                System.out.println("Too low. Guess again.");
            }
            return false;
        }
    }

    /*
     * Method to keep playing or finish it.
     */
    static public boolean playAgain(Scanner keyboard) {
        char playAgain;
        System.out.println("Play again? y/n");
        playAgain = keyboard.nextLine().charAt(0);
        while (playAgain != 'y' && playAgain != 'n') {
            System.out.println("Please use 'y' for yes or 'n' for no.");
            System.out.println("Play again? (y/n)");
            playAgain = keyboard.nextLine().charAt(0);
        }
        if (playAgain == 'n') {
            System.out.println("Thanks for playing. Good bye!");
            return true;
        } else {
            System.out.println("Let's play again!");
            return false;
        }
    }
}
share|improve this question

2 Answers 2

up vote 7 down vote accepted

First of all: I appreciate the clarity of your code. Variables have good names which causes the code to be read very fluently. Intendation, spacing and whitelines are also very well done so that's a major plus.

Ofcourse, there are always working points. Let's take a look.

Comments

// This section will be about comments

There, I clarified what this section was about. Well, actually I didn't because the title already conveyed that entire meaning. Which is exactly what you're doing with

// This is where we store the number to guess
static int numberToGuess;

or

// We initialize the score (tries)
tries = 1;

Static

You're abusing the fact that the main method should be static and bypassing the OO rules of creating an object. Instead of using that main method directly as a part of your program, simply create an object of your surrounding class (GuessGame) and use instance variables instead.

Additional benefit: you can now start multiple games at the same time.

Static helper methods

A major advantage of a helpermethod is its ability to be called in a static context (aka: Utils.doSomething()). By forcing the user to create an instance of it, you're losing that usability.

Aside from that, it also makes no sense to do that. Your helpermethod doesn't keep track of the state of anything, so there is never a difference between two instances.

Lastly: stay consistent. 1 of your 4 util methods is an instance method.

Linesplits

You don't have to split this up:

String playerName;
System.out.println("Hi there! What's your name?");
playerName = keyboard.nextLine();

This is just fine and more readable because it doesn't force your brain to look into two places for no reason:

System.out.println("Hi there! What's your name?");
String playerName = keyboard.nextLine();

Helper methods

Are they really necessary? The concept of helper methods is that they perform some tasks that can be used throughout multiple places in your project. You have one class so it's in fact impossible to have that situation.

But even if you did have multiple: these methods are all specific to that one class so they belong as members to that class.

Exceptions

This line with invalid input, will cause your program to crash. You should add appropriate exception handling:

return Integer.parseInt(keyboard.nextLine());

Nesting

This code is very nested:

if(condition) {
    return true;
} else {
    if(condition) { 

    }
}

I prefer a style that takes away some of this nesting (and thus allows more characters on your line):

if(condition) { 
    return true;
}

if(condition) {

}

Indentation

I told you before that I liked it, but after closer inspection I believe it's actually too far indented. There's an uncomfortable long white area:

public int getRandomNumber(int min, int max) {
        return min + (int) (Math.random() * ((max - min) + 1));
}

Indentation is done with 4 spaces (or 1 tab, consisting of 4 spaces). It's best to stick to these conventions.

Order of attributes

I see your methods are declared as

static public boolean isNumberCorrect()

It might be functionally the same, but once more popular convention dictates this instead (refer to the standard main method notation):

public static boolean isNumberCorrect()

I would back it up with the actual Java conventions, but for some reason that document has been offline for almost a few weeks by now.

Redundancy

"Please use 'y' for yes or 'n' for no."
"Play again? (y/n)"

This seems a little redundant. I suggest either removing the entire first line or, if you really want to keep it, to remove the "y/n". But people know by now what "y/n" means so I'd prefer to simply remove the first line.

Boolean is boolean

When I look at a variable named playAgain, I expect it to tell me "yes" or "no". I don't expect it to possibly tell me "b".

Therefore I suggest changing it to this:

boolean playAgain = keyboard.NextLine().toLowerCase().startsWith("y");

Notice how I also added toLowerCase() so "Y" is also accounted for, and used startsWith() to make the code more descriptive.

It's ongoing

This is a very minor remark so feel free to ignore the advice it if you think it isn't important.

Your while loop does this:

while (!gameEnded)

Maybe it's just me, but I'm a nitpicker and it doesn't have much feng shui to it. I would prefer to read

while(!endOfGame)

or

while(running)

Keep it short but readable

We can shorten this exhaustive block

if (tries == 1) {
    System.out.println("Good job. You guessed it in 1 try.");
} else {
    System.out.println("Good job. You guessed it in " + tries
                            + " tries.");
}

to

System.out.println("Good job. You guessed it in " + tries + (tries == 1 ? " try." : " tries.")); 

For such a small sentence, I would allow inlining that multiplication. The meaning is still conveyed clearly.

share|improve this answer
    
I would also add trim() to remove blank spaces: boolean playAgain = keyboard.NextLine().trim().toLowerCase().startsWith("y"); –  user40614 Apr 13 at 13:53
    
Just one minor thing tho, how could I use the try/catch block when I ask for a number? –  facundop Apr 14 at 11:59
    
try { Integer.ParseInt(keyboard.nextLine()); } catch (NumberFormatException e) { System.out.println("This is not a valid number } –  Jeroen Vannevel Apr 14 at 13:03
    
Thank you.. it seems to catch the first error, but if continue using bad input it crashes, I'll do some research on exceptions! I have improved the code a lot, thanks! –  facundop Apr 14 at 13:26
    
For some reason I do not like: System.out.println("Good job. You guessed it in " + tries + (tries == 1 ? " try." : " tries.")); –  Koray Tugay Apr 20 at 10:34

Not a bad start at Java! :) Below are some comments that I hope can help you improve.

Some big things:

  1. Comments: I think you make the very uncommon mistake of having too many comments rather than not enough. I'm actually loathe to even mention this, because I love when people comment well (and other people will love you for it, too), but commenting every single line is a bit overkill. For example, having a comment to explain that String playerName holds the player's name is redundant. Sometimes, it's okay for code to be self-documenting. (Indeed, some would argue that the best code is self-documenting with well-chosen variable/method names and good control structure.) Also, make sure your comments have consistent style. For some of them, you have just a description (Scanner to handle the input) while with others you have complete sentences (This is where we store the number to guess). Just having the description works fine and is less wordy, but regardless, choose a style and stick with it. Consistency is very important.
  2. Access Modifiers: All of your global variables are simply declared as static, but they should almost always be explicitly public, private, or protected.
  3. Utility Class: I like that you wanted to separate out some of your functionality into different classes. But a true "utility" class is one which has "helper methods". These could be things like testing to see if a String holds a valid int, or validating that a user's input actually exists. Instead, you have functionality which is critical to the game in the utility class. As such, those methods would be better off in the GuessGame class, since they really are part of the game. If you still want to separate out functionality, consider having a class called something like GuessGameDriver which has your main(String[] args) method, and then having your game logic in a different class. That's actually a fairly common practice.
  4. Intuitive method names/functions: The names of your methods don't always match up with what the methods actually do. I'll explain more about that below.

Some specifics:

static public boolean isNumberCorrect(int numberToGuess, int guessedNumber) {
    if (numberToGuess == guessedNumber) {
        return true;
    } else {
        if (numberToGuess < guessedNumber) {
            System.out.println("Too high. Guess again.");
        } else {
            System.out.println("Too low. Guess again.");
        }
        return false;
    }
}

I'm going to use this above method to point out a couple of things that occur throughout your code. The first is minor: according to Java convention, your access modifier should come before the static keyword (e.g., public static boolean isNumberCorrect). Not a big deal, but something to be aware of.

More significantly, your method violates the "do one thing and do it well" policy. The method is called isNumberCorrect and returns a boolean. That means it should evaluate the two numbers and check to see if the number is correct -- and nothing else. Instead, you're using this method to also provide feedback to the user. If you published your API and I wanted to use it, this would in no way be intuitive.


static public boolean playAgain(Scanner keyboard) {
    char playAgain;
    System.out.println("Play again? y/n");
    playAgain = keyboard.nextLine().charAt(0);
    while (playAgain != 'y' && playAgain != 'n') {
        System.out.println("Please use 'y' for yes or 'n' for no.");
        System.out.println("Play again? (y/n)");
        playAgain = keyboard.nextLine().charAt(0);
    }
    if (playAgain == 'n') {
        System.out.println("Thanks for playing. Good bye!");
        return true;
    } else {
        System.out.println("Let's play again!");
        return false;
    }
}

This is a curious method. All method names should be verb phrases, firstly, and if this is taken as a verb phrase then it means you are executing the code to play again. But the really curious thing about it is this: you return true if the user does not want to play again, and false if they do want to play again. That's bizarre and completely counterintuitive.


static public int guessNumber(Scanner keyboard) {
    System.out.println("Pick a number: ");
    return Integer.parseInt(keyboard.nextLine());
}

You don't have any input validation here. Meaning if I type in abjafnlsflsd, your program will break. Not a big deal for a beginner's program, but you can start to look into Exception handling as you progress.


public int getRandomNumber(int min, int max) {
    return min + (int) (Math.random() * ((max - min) + 1));
}

Just wanted to say that this is a perfect example of the kind of method that belongs in a "utility" class. It's completely divorced from the actual game and really is just a helper method. Great.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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