Prompting for a positive integer and validating user input in Java

Question

I'm just starting out with Java, and trying to make a method to get a positive integer input from the console. My currently working implementation is this:

public static Scanner sc = new Scanner(System.in);

public static int getPositiveIntInput(String message) {

    int n;

    String error_message = "Error: input must be a positive integer.";

    System.out.print(message);
    while (true) { // This is often frowned upon?

        // Check that input is integer
        while (!sc.hasNextInt()) {

            System.out.print(error_message + "\n" + message); // Gets repeated
            sc.next();

        }

        n = sc.nextInt();
        sc.nextLine();

        // Check that input is positive
        if (n > 0) {

            return n;

        } else {

            System.out.print(error_message + "\n" + message);

        }
    }

}

In order to make my code more readable and concise, I attempted to rid myself of the while (true) and several error message prints by combining the integer and positivity checks into one loop, but I'm not getting the behaviour I would expect:

public static int getPositiveIntInput(String message) {

    String error_message = "Error: input must be a positive integer.";

    System.out.print(message);

    // Check that input is integer and positive
    while (!sc.hasNextInt() || sc.nextInt() <= 0) {

        System.out.print(error_message + "\n" + message);
        sc.next();

    }

    int n = sc.nextInt();
    sc.nextLine();

    return n;

}

---

What would be an effective way of improving the readability and reducing repetition while achieving the same functionality as in my original implementation? Particularly regarding the while(true) loop - I've seen it's use being discouraged in other SO threads. Would this be a case where a different looping structure could/should be considered?

Answer 1

while (true) { // This is often frowned upon?

Frowned upon? Not that I know of.

Using a scanner for extracting numbers or tokens from a stream of input is great when the data is guaranteed to be in the expected format, and when it's ok to crash on any error.

The .nextInt and similar methods are less practical when you have to validate the input and give a chance to retry. In such cases I find it a lot more practical to read line by line, and perform the integer parsing manually, like this:

public static int getPositiveIntInput(String message) {    
    System.out.print(message);

    while (true) {
        String line = sc.nextLine();
        try {
            int n = Integer.parseInt(line);
            if (n > 0) {
                return n;
            }
        } catch (NumberFormatException e) {
            // ok to ignore: let it fall through to print error message and try next line
        }
        System.out.print("Error: input must be a positive integer.\n" + message);
    }
}

Answer 2

First of all, let me review your current working code. There is really not much I could improve: I find your code very readable and clear. Still, a couple of comments

1. You should try to respect the Java naming conventions. In this case the variable error_message should be named errorMessage: camel-case is preferred.

2. Try to always declare the variables when they are needed. In this case, the int n at the start of the method can be moved further down so that you have

   int n = sc.nextInt();
   // rest of code
   

3. If you input something with a space, you will have a slightly incorrect output (the error message will be printed as many times as there are spaces). This is because you are calling sc.next(); to ignore it when you should be calling sc.nextLine();. next() returns the next token and tokens are space-separated by default.

---

Your attempt is really close. You could have the following:

public static int getPositiveIntInput(String message) {
    int n;
    String errorMessage = "Error: input must be a positive integer.";
    System.out.print(message);
    while (!sc.hasNextInt() || (n = sc.nextInt()) <= 0) {
        System.out.print(errorMessage + "\n" + message);
        sc.nextLine();
    }
    sc.nextLine();
    return n;
}

What this does is that it enter the while loop to display the error message when either the next token of the scanner isn't an int or the returned value of nextInt() isn't strictly positive. The trick here is to store the result of call directly in the while expression.

If this is better, I'm not sure though... :)