6
\$\begingroup\$

I am a beginner to Java and have just written a number guessing game which involves guessing numbers from 1 to 10. I would like to improve my code by sticking strictly to conventions, improving code efficiency and forming good coding habits from the beginning.

Main class:

public class Main {

    public static void main(String[] args) {
        while (GuessingGame.play) {
            GuessingGame.play();
        }
    }
}

GuessingGame class:

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

public class GuessingGame {

    public static boolean play = true;

    private static int number = 0;
    private static int guess = 0;
    private static Random random = new Random();
    private static Scanner scanner = new Scanner(System.in);

    private static int nextInt(Random r, int lower, int higher) {
        int ran = r.nextInt(higher - lower);
        return (int) lower + ran;
    }


    public static void play() {
        number = nextInt(random, 1, 10);
        System.out.println("Insert your guess from 1-10!");
        guess = scanner.nextInt();
        if (guess == number) {
            System.out.println("Correct!");
            play = false;
        } else {
            System.out.println("Wrong! The number is: " + number);
            System.out.print("Do you want to try again? Y/N");
            String s = scanner.next();
            play= s.equalsIgnoreCase("Y") ? true : false;
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

There's no point in writing a separate class just to contain the main() function. I'd rather invoke java GuessingGame than java Main.

while (GuessingGame.play) is bad in several ways:

  • GuessingGame exposes a public variable. It's standard practice to require Main to call a method, so that no other class can write to a variable in the GuessingGame class.
  • The method GuessingGame.play() and the variable GuessingGame.play are too similarly named, which is confusing.
  • GuessingGame.play() sets the GuessingGame.play flag as a side-effect, which is unconventional and unintuitive. It also violates the Single Responsibility Principle: play() should do one thing only.
  • A do-while loop might be more appropriate, since you want to play at least one game.

In GuessingGame, everything is static. The number and guess variables could easily be made into local variables in play(), and therefore they should be.

Since the Random object is a class variable, there is no point in passing it as a parameter to nextInt(). There is a bug in nextInt(), in that it will never generate a 10.

Suggested solution

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

public class GuessingGame {

    private static Random random = new Random();
    private static Scanner scanner = new Scanner(System.in);

    /**
     * Picks a number uniformly between lowerBound (inclusive)
     * and upperBound (exclusive).
     */
    private static int nextInt(int lowerBound, int upperBound) {
        return lowerBound + random.nextInt(upperBound - lowerBound);
    }

    /**
     * Plays one game.
     *
     * @return true if the guess was correct
     */
    public static boolean play() {
        int number = nextInt(1, 10 + 1);
        System.out.println("Insert your guess from 1-10!");
        if (scanner.nextInt() == number) {
            System.out.println("Correct!");
            return true;
        } else {
            System.out.println("Wrong! The number is: " + number);
            return false;
        }
    }

    /**
     * Asks if the user wants to play again.
     */
    public static boolean shouldPlayAgain() {
        System.out.print("Do you want to try again? Y/N ");
        return "Y".equalsIgnoreCase(scanner.next());
    }

    /**
     * Plays guessing games until the user guesses correctly or gives up.
     */
    public static void main(String[] args) {
        while (!play() && shouldPlayAgain());
    }
}
\$\endgroup\$
0
\$\begingroup\$
  • Why have you made these methods static?

    In this case if multiple users try to play the same game at the same time then variable number and guess shared by all thread, which is not desired because every played has its own number and own guess.

  • In this code:

    play= s.equalsIgnoreCase("Y") ? true : false;
    

    equalsIgnoreCase is already returning true and false, so you need not check true and false and set the value based on the same.

    Your code should be:

    play= s.equalsIgnoreCase("Y");
    
  • In this code:

    number = nextInt(random, 1, 10);
    System.out.println("Insert your guess from 1-10!"); 
    

    If you are assuming that the range may vary, then take the range as an input argument in the method and use the same in your printing message:

    public void play(int n, int m) { 
        number = nextInt(random, n, m); 
        System.out.println("Insert your guess from " + n + "-" + m + "!"); 
    
\$\endgroup\$
  • \$\begingroup\$ The static keyword was added because I had to call the method play() from the Main class in the static main method. Is it possible to not use the static keyword in this case? \$\endgroup\$ – user1476382 Jun 19 '15 at 4:43
  • \$\begingroup\$ Instead of static create an object and then call play method. \$\endgroup\$ – Ashish Aggarwal Jun 19 '15 at 4:53
  • 2
    \$\begingroup\$ static does not mean "shared by all thread", and has nothing to do with 1 or more players at a time. \$\endgroup\$ – rolfl Jun 19 '15 at 5:01
  • \$\begingroup\$ Yes static means directly loaded into the memory and in this case it has global data on which he is playing In programming like guess and number. if mutliple threads trying at the same time then number and gusswill create problem because shared by all threads. \$\endgroup\$ – Ashish Aggarwal Jun 19 '15 at 5:04
  • 3
    \$\begingroup\$ Global data is a problem in single threaded applications too. No need to drag threads into this. Static is a problem because it's global data and as such prevents multiple independent instances (regardless of threads). Static should not be taken lightly, use when there is a good reason. Here there's no good reason. Focus on that, forget about threads \$\endgroup\$ – janos Jun 19 '15 at 5:34

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.