Java guess the number

Question

I have just returned to college to finish my associate's in programming. The language my school uses for all of the programming classes is java, so I've been trying to get used to the language.

The code I've posted below works as far as I can tell, I am hoping some of you can give some constructive criticism to my code as far as the style and general improvements I could have made.

I am very new to the OOP concept, so I am also wondering if I declared everything right, I know global variables are normally bad practice, but since the global variables I used are in a class I wasn't sure if that was really the same thing since they are all local variables anyway.

Also, the main function isn't included here because all it does is call the public function playGame(), but if you really need it I can add it later.

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

/**
 * Title: GameFunctions
 * Author: Dev
 * Date: 6/1/2019
 * Purpose: This class contains -
 * -all the functions for the game
 * */

public class GameFunctions
{
    private static int attempts;
    private static int randomNumber;
    private static int guessRange;
    private static int userGuess;

    private static boolean gameRunning = true;

    /*
     *  Takes care of the setup aspects of the game like
     *  Generating a new random number and finding how
     *  Many attemps the player will have
     */
    private static void gameInit()
    {
        setRandomNumber();
        setAttemptsAndRange();
    }

    /* Sets randomNumber to a value between 1 and 100 */
    private static void setRandomNumber()
    {
        Random random = new Random();
        int low = 0;
        int high = 100;
        randomNumber = random.nextInt(high - low) + low;
    }

    /* Sets the attempts value based on how large the randomNumber is */
    private static void setAttemptsAndRange()
    {
        if (randomNumber <= 10) {attempts = 2; guessRange = 10; }
        else if (randomNumber <= 25) { attempts = 4; guessRange = 25; }
        else if (randomNumber <= 50) { attempts = 5; guessRange = 50; }
        else if (randomNumber <= 75) { attempts = 7; guessRange = 75; }
        else { attempts = 10; guessRange = 100; }
    }

    /* Displays the number of attempts the user has left */
    private static void getAttempts()
    {
        System.out.println("Attempts remaining: " + attempts);
    }

    /* Displays the random number, used at the end of the game */
    private static void getRandomNumber()
    {
        System.out.println("The random number is " + randomNumber);
    }

    /* Gets input from the user */
    private static void getInput()
    {
        Scanner scanner = new Scanner(System.in);

        while (true)
        {
            try
            {
                getAttempts();
                System.out.println("Enter a number between 0 and " + guessRange);
                userGuess = scanner.nextInt();

                if ( userGuess <= 100 && userGuess > 0) { break; }
                else { System.out.println("Number is out of guess range"); }
            }
            catch (java.util.InputMismatchException e)
            {
                System.out.println("Invalid input");
                scanner.nextLine();
            }
        }
    }

    /*
     * Checks to see if the guess is correct
     * If it is not correct it will decrement the attempts value
     */
    private static void checkGuess()
    {
        if (userGuess != randomNumber)
        {
            if (userGuess < randomNumber) { System.out.println("Too Low!"); }
            else { System.out.println("Too High!"); }
            attempts--;
        }
    }

    /* Tells the user their win/loss status and asks them to play again */
    private static void playAgain()
    {
        String userInput;
        Scanner scanner = new Scanner(System.in);

        if (attempts > 0) { System.out.println("You won!"); }
        else { System.out.println("You Lost!"); };

        getRandomNumber();

        while (true)
        {
            try
            {
                System.out.println("Play Again? yes/no");
                userInput = scanner.next();
                if (userInput.equals("yes") || userInput.equals("no"))
                {
                    if (userInput.equals("yes")) { break; }
                    else { gameRunning = false; break; }
                }
                else { System.out.println("Invalid Input"); }
            }
            catch (java.util.InputMismatchException e)
            {
                System.out.println("Invalid input, try again");
            }
        }

    }

    /* Main game loop */
    public static void playGame()
    {
        while (gameRunning)
        {
            gameInit();
            while (attempts > 0 && userGuess != randomNumber)
            {
                getInput();
                checkGuess();
            }
            playAgain();
        }
    }
}

Answer 1

Let's take this from the top down:

/**
 * Title: GameFunctions
 * Author: Dev
 * Date: 6/1/2019
 * Purpose: This class contains -
 * -all the functions for the game
 * */
public class GameFunctions

This comment doesn't tell me anything new:

• Title is already mentioned in the class declaration that follows
• Author and Date are already kept in version control software
• Purpose simply restates the class name

I recommend writing a useful comment, or none at all.

{

All major coding standards for Java put opening braces at the end of the preceeding line, not a line of their own.

private static int attempts;
private static int randomNumber;
private static int guessRange;
private static int userGuess;

Keeping mutable state in static fields is unusual because it needlessly inhibits code reuse (prevents several instances of the game from running concurrently), and is somewhat verbose due to having to repeat static everywhere. Consider using non-static fields instead.

/*
 *  Takes care of the setup aspects of the game like
 *  Generating a new random number and finding how
 *  Many attemps the player will have
 */
private static void gameInit()

Such initialization logic is usually expressed in a constructor, because that enforces that state is initialized before use (you can forget calling an init method, but the compiler will yell at you if your forget to call a constructor).

Your javadoc comment restates the implementation in its entirety. Not only is this redundant, it also violates information hiding: Javadoc is intended to inform the callers of this method about its proper use. A caller doesn't need to know which private fields are being set, and how. All a caller needs to know is when he needs to invoke this method. Since this is already being conveyed by the method name, a comment is superfluous.

   setRandomNumber();
   setAttemptsAndRange();

As a matter of style, prefer expressing yourself in the programming language instead of through clever names (names are only understood by humans, and can not be checked by your compiler, and refactored by your refactoring tools).

That is, I'd have written:

randomNumber = randomInteger(100)
guessRange = rangeOf(randomNumber)
attempts = maximumAttemptsFor(randomNumber)

Passing randomNumber allows both the compiler and future maintainers to see and check that randomNumber is initialized before use.

/* Displays the number of attempts the user has left */
private static void getAttempts()

The Javadoc contradicts the method name: Do we "get" or "display" the number of attempts?

/* Gets input from the user */
private static void getInput()

Actually, it also validates the input. A caller might want to know that he doesn't have to check the range.

Perhaps rename the method to readGuess()? That makes it clear that only guesses are returned.

* Checks to see if the guess is correct
* If it is not correct it will decrement the attempts value
*/
private static void checkGuess()

The method implementation also displays feedback about the guess. A caller might want to know that.

/* Tells the user their win/loss status and asks them to play again */
private static void playAgain()

The Javadoc and method name seem at odds. What does "playing again" have to do with a win/loss status?

/* Main game loop */
public static void playGame()
{
    while (gameRunning)
    {
        gameInit();
        while (attempts > 0 && userGuess != randomNumber)
        {
            getInput();
            checkGuess();
        }
        playAgain();
    }
}

Well done! By structuring your program into steps, each in their own named method, the main method gives a very good overview of your program, and makes it very easy and quick to drill down to a particular part of the code.

However, why is this excellent summary at the end of your source file? Wouldn't it do more good if it were the first thing the reader of this class sees?

And a last nitpick: your summary reads a variable (gameRunning), but doesn't express when that variable is set. This could be seen a mixing layers of abstraction.

All that said, here is a sketch how I'd write it:

public class GuessingGame {

    public static void main(String... args) {
        do {
            new GuessingGame().play();
        } while (playAgain());
    }

    private final int secretNumber;
    private final int guessRange;
    private int remainingAttempts;

    public GuessingGame() {
        secretNumber = randomInteger(100);
        guessRange = guessRange(secretNumber);
        remainingAttempts = maximimumAttempts(secretNumber);
    }

    public void play() {
        while (remainingAttempts > 0) {
            remainingAttempts--;

            int guess = readGuess();
            if (guess == secretNumber) {
                display("You won!);
                return;
            }
            display(hint(guess));
        }
        display("You lost!");
    }

    // more utility methods go here
}

PS: This review held your code to professional standards. For a beginner, you did very well! Should you want to learn more about writing maintainable code, I can recommend "Clean Code" by Robert C. Martin - he may be a bit dogmatic at times, but his arguments are well worth considering.

Answer 2

I have some observations.

1. All your methods & fields are static: This is not required. You can remove the static modifier.

2. Getter Methods:

   private static void getAttempts()
   {
       System.out.println("Attempts remaining: " + attempts);
   }
   

A getter method is expected to return some value and not print some value. If you want to print attempts then rename method to printAttempts().

3. Formatting in if/else blocks: No formatting in if/else block of your code

   if (randomNumber <= 10) {attempts = 2; guessRange = 10; }

should be

if(randomNumber <= 10){
      attempts = 2;
      guessRange = 10;
}

Answer 3

All the other answers are very good and detailed but they fail to touch upon the reasoning behind the fatal flaw in your code: static variables are generally bad and to be avoided. This answer should help you understand some of the basics of static variables so you can understand the other answers better. To explain that we need to touch on some OOP basics and what a static variable is in a class.

The principle of OOP is that you can create objects containing their own methods and variables so that all of the code relating to a certain task is in an "object". In java an "object" is created in the form of a class.

The main benefits of OOP are twofold, it simplifies the code, and more importantly allows effective code reuse and duplication. Static variables and methods are, generally speaking, against code reuse and duplication. A class is meant to be initialized before using, essentially making an instance of the class before you use it. In java this initialization would look like this:

GameFunctions game = new GameFunctions();

And then all methods after that would be called like

game.getInput();

This is very useful because it allows multiple instances of a class to be created simultaneously so if you wanted to have two games running you could do something like:

GameFunctions game1 = new GameFunctions();
GameFunctions game2 = new GameFunctions();

Static variables, instead of being owned by each object as you create them are owned by the class itself, in other words there will only really be one GameFunctions object. This object may work just fine, but it's far better to do things in terms of non static variables. Although static variables do have their place, that place is very rare and this class shouldn't have a single static variable.

That being said, this is a common mistake everyone makes when they first start, your code seems pretty well thought out and I'm sure you're on track to learn much more in the way of coding!

Answer 4

In addition to Himanshu's answer.

Move the instantiation of Random to the class level.

    private static void setRandomNumber()
    {
        Random random = new Random();
        int low = 0;
        int high = 100;
        randomNumber = random.nextInt(high - low) + low;
    } 

You are creating new instance of Random every time you call the method. Move the instantiation to the class level and only call it from the method, otherwise you are creating objects that are used only once.

Answer 5

I do not know if it is a good tip ( I am a beginner too), but I think it would be a better practice to initialize other classes at the very top using constructor.

You instantiate scanner twice, together with removing all static modifiers, you could instantiate scanner and random automatically when you instantiate GameFunctions class in main class.