5
\$\begingroup\$

In this exercise I have to do this:

Modify the program so that no matter what number the user thinks of (1-100) the computer can guess it in 7 or less guesses.

I'm just a beginner in Java and I'd like to know what you think about my code:

public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    Random rand = new Random();
    int randNum = 0;
    int upperLimit = 100;
    int lowerLimit = 1;
    String myAnswer = "";

    do {
        randNum = rand.nextInt(upperLimit - lowerLimit + 1) + lowerLimit;
        System.out.println("I think it's " + randNum);
        myAnswer = in.nextLine();   

        if(myAnswer.equals("tl")) {  //too low
            lowerLimit = randNum + 1;
        } 
        else if(myAnswer.equals("th")) { // too high
            upperLimit = randNum - 1;
        }   
    }while(!myAnswer.equals("y"));

    in.close();
    System.out.println("YAAAY! :D");
}

I think it's too slow, because sometimes it takes the computer more than 7 times to guess a number.

\$\endgroup\$
0
10
\$\begingroup\$

The guessing takes too long because picking a random number is not the best idea. For example if the number to guess is 100, and the computer is really really unlucky, it might pick the numbers 1, 2, 3, 4, 5, 6, ... and take 100 guesses!

A better way is to halve the intervals. That is, if you know the number must be between 1 and 100, then pick 50. If that's too high, and so you know the number is between 1 and 50, then pick 25. And so on.

So, changing one line could make a huge difference:

    // randNum = rand.nextInt(upperLimit - lowerLimit + 1) + lowerLimit;
    randNum = (upperLimit + lowerLimit + 1) / 2;

This is called a binary search. As @cbojar pointed out in comments, since binary search takes \$\log_2(n)\$ tests where \$n\$ is the total number of objects, and since \$\log_2(100) = 6.643856\$, that means it's guaranteed that this algorithm will find the correct number in at most 7 steps.


The string literals "th", "tl", "y" that control the flow of the program are buried inside the code. If you read this program a month from now, it might not be obvious how it works until you read the details. Something that would help with that, and a good practice is to extract these string literals to constants, for example:

private static final String TOO_HIGH_MARKER = "th";
private static final String TOO_LOW_MARKER = "tl";
private static final String CORRECT_MARKER = "y";

// ...

public static void main(String[] args) {
    // ...

    do {
        // ...

        if (myAnswer.equals(TOO_LOW_MARKER)) {  //too low
            lowerLimit = randNum + 1;
        } else if (myAnswer.equals(TOO_HIGH_MARKER)) { // too high
            upperLimit = randNum - 1;
        }

        // ...
    } while (!myAnswer.equals(CORRECT_MARKER));

Now all these magical strings are easy to see near the top. Should you decide to use different values, you could just edit them right at the top, simple and clear.

UPDATE

As an extra touch, as @Tom pointed out in a comment, it might be a good / interesting idea to make these equals comparisons a bit more fault tolerant by making them case insensitive by changing to equalsIgnoreCase, for example:

        if (myAnswer.equalsIgnoreCase(TOO_LOW_MARKER)) {  //too low

This is of course less efficient than a simple equals, so don't use it excessively everywhere, only when justified.


These initializers are redundant:

int randNum = 0;
String myAnswer = "";

Because, these variables will always be assigned before they are used. As such, you can remove the initializers:

int randNum;
String myAnswer;

It's a cosmetic thing, but your formatting doesn't follow the standard in a few places.

Instead of this:

if(myAnswer.equals("tl")) {  //too low
    lowerLimit = randNum + 1;
}
else if(myAnswer.equals("th")) { // too high
    upperLimit = randNum - 1;
}

It should be like this:

if (myAnswer.equals("tl")) {  //too low
    lowerLimit = randNum + 1;
} else if (myAnswer.equals("th")) { // too high
    upperLimit = randNum - 1;
}

And instead of this:

}while(!myAnswer.equals("y"));

It should be like this:

} while (!myAnswer.equals("y"));

Again, these are minor issues, but as your programs get larger and larger these little details can add up and seriously hurt readability. Use editor environments like Eclipse or IntelliJ or Netbeans that can automatically reformat your code following the standard.

\$\endgroup\$
6
  • 1
    \$\begingroup\$ Maybe it is a good idea to recommend using myAnswer.toLowercase().equals(TOO_LOW_MARKER) or myAnswer.equalsIgnoreCase(TOO_LOW_MARKER) to avoid problems with case sensitivity. \$\endgroup\$ – Tom Nov 9 '14 at 22:16
  • 3
    \$\begingroup\$ It is mathematically provable because this is doing a binary search over the range of numbers. Binary search takes (base 2) log(n) tests where n is the total number of objects. log(100) is just less than 7. \$\endgroup\$ – cbojar Nov 9 '14 at 22:50
  • 2
    \$\begingroup\$ What is "the" formatting standard? What is this nonsense?! For the record, I disagree with yours. \$\endgroup\$ – Lightness Races in Orbit Nov 9 '14 at 22:58
  • \$\begingroup\$ @LightnessRacesinOrbit feel free to disagree! This is not "mine" though. This is what Eclipse, IntelliJ NetBeans do when you use their auto-reformat feature. \$\endgroup\$ – janos Nov 10 '14 at 6:30
  • \$\begingroup\$ Thanks @cbojar, clarified and improved my answer with that! \$\endgroup\$ – janos Nov 10 '14 at 6:44
4
\$\begingroup\$

I'm not sure about performance, but here's a quick review on other things:

  • Instead of declaring randNum outside the loop, just initialize it inside:

    int randNum = rand.nextInt(upperLimit - lowerLimit + 1) + lowerLimit;
    

    Try to keep variables as close in scope as possible (not "listing" all of them).

  • This output is noisy:

    System.out.println("YAAAY! :D");
    

    If the user has indeed guessed the correct number, then just state that.

\$\endgroup\$
0

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