11
\$\begingroup\$

The program is asking you to guess a number between 1-100 and if you guessed too low it says that in Swedish and the same goes for if it's too high and if it's the correct answer it says congratz you guessed right in Swedish.

I'd like comments about any aspects of my code.

public class Kidsprogram {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        Random rand = new Random();
        int number = rand.nextInt(100) + 1;
        System.out.println("Gissa på ett nr mellan 1 och 100");
        for (int i = 1; i > -1; i++)
        {
            int guess;
            guess = input.nextInt();
            if (guess < number)
            {
                System.out.println("Du gissade för lågt");
            }
            else if(guess > number)
            {
                System.out.println("Du gissade för högt");
            }
            else if(guess == number)
            {
                System.out.println("Antal gissningar: " + i);
                System.out.println("Grattis du gissade rätt!");
                break;
            }
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

Scanner is an auto-closable resource, an it should be closed. Your code is probably showing a warning for that in your IDE. The following should help:

try (Scanner scanner = new Scanner(System.in)) {
    ... put your program here.
}

While the close on the System.in is often ignored, when putting a Scanner on it, you should do it to remove the warnings.

Function extraction is a good thing, and when dealing whith human interaction, you should be defensive about handling user input. What happens if the user types "foo", instead of a number?

A method like:

public static int readInt(Scanner scanner) {
    while (!scanner.hasNextInt()) {
        System.out.println("Det är inget tal. Ange ett värde (1-100):");
        scanner.next();
    }
    return scanner.nextInt();
}

will loop until there is a valid input.

Your main mentod is still doing too much, though, your should probably extract the random number too. Consider:

private static int randomInt(int from, int to) {
    Random rand = new Random();
    return rand.nextInt(to - from + 1) + from;
}

Then you can call:

int number = randomInt(1, 100);

Finally, you can outsource the whole thing to:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        System.out.println("Guess!");            
        int turns = countTurns(scanner, randomInt(1, 100));
        System.out.println("Antal gissningar: " + turns);
        System.out.println("Grattis du gissade rätt!");
    }
}

if you have a countTurns method:

private static int countTurns(Scanner scanner, int number) {
    int turn = 0;

    while (true) {
        turn++;
        int guess = readInt(scanner);
        if (guess < number) {
            System.out.println("Du gissade för lågt");
        } else if (guess > number) {
            System.out.println("Du gissade för högt");
        } else {
            return turn;
        }
    }
}

public static int readInt(Scanner scanner) {
    while (!scanner.hasNextInt()) {
        System.out.println("Det är inget tal. Ange ett värde (1-100):");
        scanner.next();
    }
    return scanner.nextInt();
}

private static int randomInt(int from, int to) {
    Random rand = new Random();
    return rand.nextInt(to - from + 1) + from;
}

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        System.out.println("Guess!");            
        int turns = countTurns(scanner, randomInt(1, 100));
        System.out.println("Antal gissningar: " + turns);
        System.out.println("Grattis du gissade rätt!");
    }
}
\$\endgroup\$
8
\$\begingroup\$

Since the end condition of the loop is always true, you can just omit it:

    for (int i = 1; ; i++)

The condition in the final else if(guess == number) is always true, so you can use simply else.

There's no need to separate the declaration and initialization of the guess variable, you can do int guess = input.nextInt() which is shorter.

The convention in Java is to put opening braces on the same line as the statement, like this:

    for (int i = 1; ; i++) {
        int guess = input.nextInt();
        if (guess < number) {
            System.out.println("Du gissade för lågt");
        } else if(guess > number) {
            System.out.println("Du gissade för högt");
        } else {
            System.out.println("Antal gissningar: " + i);
            System.out.println("Grattis du gissade rätt!");
            break;
        }
    }
\$\endgroup\$
3
\$\begingroup\$
  1. Put Scanner scanner = new Scanner(System.in) in a try block to safely close the input stream upon termination.

  2. Consider declaring int guess = 0 outside of the loop, and use that for the end condition of the for loop:

    int guess = 0;
    for (int i = 1; guess != number; i++) {
        ...
    }
    
  3. Since the output for a wrong guess is largely similar, you can also consider experimenting with the ternary operator to determine the String you want to display. The minor downside to this is that implicitly, you have a nested if statement instead of the one if-else-if-else flow now.

Putting them together:

    int guess = 0;
    for (int i = 1; guess != number; i++) {
        guess = input.nextInt();
        if (guess != number) {
            System.out.println("Du gissade för " + (guess < number ? "lågt" : "högt"));
        } else {
            System.out.println("Antal gissningar: " + i);
            System.out.println("Grattis du gissade rätt!");
        }
    }
\$\endgroup\$
3
\$\begingroup\$
  1. Main method
    Your main method is doing too much. In any program, the main method should be the controller rather then the executor. You can remove parts such as the random number generation into other methods and call them from main. Your main method should just tie all the other methods together and manage user input and output.

  2. Scanner
    Scanner should be put in a try block, as others have said, to make sure it is closed.

    try(Scanner scanner = new Scanner(System.in)) { ... }
    
  3. Loop Your for loop is just creating an infinite loop. Assuming that's what you want it to do (which, from the code, it looks like you do), you should just use a much simpler while(true) loop, with exactly the same code inside it.

\$\endgroup\$

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.