Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have two functions that are used to prompt and recieve input from the user. One is for int and the other is for boolean. They work somewhat similar to each other, one uses try/catch and the other doesn't. Can they be improved upon or is one better than the other? I'm having trouble with the user interface, I'm not sure what should happen when the user gives an invalid input. I asked on userexperience and didn't get much feed back.

/*returns integer to question asked in prompt*/
public static int getNumber(String prompt)
{
    Scanner sc = new Scanner(System.in);
    while (true) {
        System.out.print("\t"+prompt+": ");
        String input = sc.next();
        try {
            return Integer.parseInt(input);
        } catch (NumberFormatException ne) {
            System.out.println("\tPlease enter an integer.");
        }
    }
}

/*returns boolean to question asked in message*/
public static boolean getBool(String message)
{
    Scanner sc = new Scanner(System.in);
    String answer;

    System.out.print("\t"+message+": ");
    while (true) {
      answer = sc.nextLine().trim().toLowerCase();
      if (answer.equals("y")) {
        return true;
      } else if (answer.equals("n")) {
        return false;
      } else {
         System.out.print("\tPlease answer y/n");
      }
    }

}

One problem is "please enter an integer". If the user is non-technical they may not know what an integer is, but I can't think of better wording.

Here is another version of the methods

/*returns integer to question asked in prompt*/
public static int getNumber(String prompt)
{
    Scanner sc = new Scanner(System.in);
    while (true) {
        System.out.print("\t"+prompt+": ");
        String input = sc.next();
        try {
            return Integer.parseInt(input);
        } catch (NumberFormatException ne) {
            System.out.println("\tPlease enter an integer.");
        }
    }
}

/*returns boolean to question asked in message*/
public static boolean getBool(String message)
{
    Scanner sc = new Scanner(System.in);
    String answer;


    do{
      System.out.print("\t"+message+" (y/n): ");
      answer = sc.nextLine().trim().toLowerCase();
      if (answer.equals("y")) {
        return true;
      } else if (answer.equals("n")) {
        return false;
      }
    }while(true);

}

An example of these functions being used is when they are called with

public static void main(String[] args)
{

    System.out.println("Please reply with y or n"); 
    boolean celsius = getBool("Temperature in celsius"); 
    boolean wind = getBool("Regularly windy"); 
    System.out.println("Please enter an integer");
    int minTemp = getNumber("Minimum temperature during day"); 
    int maxTemp = getNumber("Maximum temperature during the day"); 
    int minTempNight = getNumber("Minimum temperature during the night"); 
    int maxTempNight = getNumber("Maximum temperature during the night");
    //setup();
}

Example input/output

run:
Please reply with y or n
    Temperature in celsius (y/n): m
    Temperature in celsius (y/n): 5
    Temperature in celsius (y/n): y
    Regularly windy (y/n): asdf
    Regularly windy (y/n): y
Please enter an integer
    Minimum temperature during day: 5
    Maximum temperature during the day: 20
    Minimum temperature during the night: blah
    Please enter an integer.
    Minimum temperature during the night: 1
    Maximum temperature during the night: words
    Please enter an integer.
    Maximum temperature during the night: 10
share|improve this question

2 Answers 2

If the user is non-technical they may not know what an integer is, but I can't think of better wording.

[Positive] Number or [Positive] Whole Number would be alternatives.

Another possibility: Enter a Number between 0 and [X]. Non-technical people will assume that this means they should enter a whole number. If they enter a different number, give feedback accordingly.

Consistency

In your first version: getNumber outputs the prompt each time, while getBool does not. This should be consistent. The names of the input variable should be consistent as well (either prompt or message. I would prefer prompt.

Other

You can do sc.nextInt(); instead of String input = sc.next(); Integer.parseInt(input);.

As already mentioned on UX, show the possible values for the boolean answer in the prompt when asking the question. So

boolean wind = getBool("Regularly windy"); 

becomes

boolean wind = getBool("Regularly windy (y/n)"); 

Extract Method

You can extract the method to retrieve a string. I think that this makes the code a bit more readable, and you get getString method for free.

/*returns integer to question asked in prompt*/
public static int getNumber(String prompt) {
    while (true) {
        String input = getString(prompt);
        try {
            return Integer.parseInt(input);
        } catch (NumberFormatException ne) {
            System.out.println("\tPlease enter a positive whole number.");
        }
    }
}

/*returns boolean to question asked in prompt*/
public static boolean getBool(String prompt) {
    while (true) {            
        String input = getString(prompt).trim().toLowerCase();
        if (input.equals("y")) {
            return true;
        } else if (input.equals("n")) {
            return false;
        } else {
            System.out.print("\tPlease answer y/n");
        }
    }
}

/*returns string to question asked in prompt*/
public static String getString(String prompt) {
    System.out.print("\t" + prompt + ": ");
    Scanner sc = new Scanner(System.in);
    return sc.nextLine();
}
share|improve this answer
    
Would it be a further improvement if getString is made private since it really isn't intended to be used by anything outside of the class? –  Celeritas Aug 6 at 21:02
    
@Celeritas Yes, it is best practice to declare internal methods as private. So if you know that you will never need to get a string as user input, then you can declare it private. But in this case, I wouldn't really mind for it to be a public method, as it fits in with the other methods quite nicely –  tim Aug 6 at 21:23

This problem has me intrigued.

There are a number of problems with your code. The repeated use of Scanners on System.in concerns me. System.in is a synchronized resource, but, what if scanners are reading them in different threads? Also, does the scanner call close() when it is finalized, and then close the System.in? I would much rather see a single control point for the code that allows only one Scanner on System.in, and does all the work for you. To accomplish that, I recommend a configurable plugin type system.

Consider an interface like:

public interface InputControl<T> {
    public String getRequest();
    public T parse(String input) throws IllegalArgumentException;
}

Then, consider your int input mechanism, and there will be only one for your whole system (a singleton):

public class UserInteraction {
    private final Scanner scanner = new Scanner(System.in);

    public final <T> T prompt(InputControl<T> control) {
        synchronized(scanner) {
            do {
                System.out.print(control.getRequest() + ": ");
                String input = scanner.nextLine();
                try {
                    return control.parse(input);
                } catch (IllegalArgumentException iae) {
                    System.out.println("Unable to process input " + input + ": " + iae.getMessage());
                }
            } while (true);
        }
    }
}

OK, so there's a system that takes a control structure, and uses it to request input. It will continue to request input until the input parses successfully.

Now, how to use it:

public class YesNoInput implements InputControl<Boolean> {
    private final String message;
    public YesNoInput(String message) {
        this.message = message + "(y/n)";
    }

    public String getRequest() {
        return message;
    }

    public Boolean parse(String input) throws IllegalArgumentException {
        final String simplified = input.toLowerCase().trim();
        switch (simplified) {
            case "y" :
                return Boolean.TRUE;
            case "n" :
                return Boolean.False;
            default:
                throw new IllegalArgumentException("Input '" + input + "' is neither 'y' nor 'n'");
        }
    }
}

You can create similar implementations for String, integer, or other custom inputs.

Now, your code simply becomes:

private static final UserInteraction UI = new UserInteraction();


.....


    boolean usecelcius = UI.prompt(new YesNoInput("Use Celcius"));

    // IntegerInput takes a minimum and maximum range,
    //    -273 celcius is 0 Kelvin, so that's the minimum....
    int value = UI.prompt(new IntegerInput("Enter temperature", -273, Integer.MAX_VALUE));
share|improve this answer
    
Why not just have private final Scanner sc = new Scanner(System.in); as a member field (like a global variable)? –  Celeritas Aug 1 at 23:28
    
@Celeritas - mostly because I would want the parse method to be global, otherwise you have to pass a scanner in to the parse method as well.... and, the class encapsulates the UserInteraction, not the Scanner. –  rolfl Aug 1 at 23:29
    
Instead of message as name I'd have used prompt. +1 either way :> –  Vogel612 Aug 1 at 23:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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