1
\$\begingroup\$

I have attemped to define a function which scan an int value in a specified range and returns that value. The lower bound is the lower of the two parameters, the upper bound is the larger of the two parameters.After invalid input, the function displays a message and repeats input (and message, if necessary ) until a valid value is entered, which is then returned.

public static int scanYear() {


    do{
    int year = TextIO.getInt();
    if (checkDate (year) == true)
        return year;
    else
        TextIO.put("year not in the allowed range");

    }while (true);

} // end of scanYear

What is your opinion about the efficiency of the code , what can be improved ? Many thanks.

\$\endgroup\$
  • \$\begingroup\$ Could you add the checkDate method as well? \$\endgroup\$ – Simon Forsberg Nov 24 '13 at 16:53
  • \$\begingroup\$ if (checkDate (year) == true) the == true is redundant \$\endgroup\$ – recursion.ninja Nov 24 '13 at 18:29
  • \$\begingroup\$ efficiency when you are waiting on user input is kinda unimportant \$\endgroup\$ – ratchet freak Nov 24 '13 at 19:54
3
\$\begingroup\$

When it comes to user-input, efficiency really is not the point.... what does it matter if it takes 4 milliseconds to validate some data, or 4 nanoseconds? The user component of time dwarfs the rest.... Obvioulsy, if the code was taking more than 10 milliseconds I would be concerned, but I doubt that is the case.

With user-interface coding though, you have to think differently. In this case, you have to help the user. Specifically, telling a user that their input is wrong is more frustrating if you don't tell the user how to correct it....

Saying: " year not in the allowed range" is going to lead to lots of frustration.

Saying: "year must be between 1950 and 2013" is much better.

EDIT: In fact, when prompting the user to enter the data, it should inform the user what values will be valid before getting the value:

Please enter a year between 1950 and 2013 (inclusive):
...
Sorry, the value 2014 is not between 1950 and 2013, please enter a valid year:
\$\endgroup\$
2
\$\begingroup\$

Not much to review since you didn't provide us much but 2 small things

  1. In Java

    if (checkDate (year) == true)
    

    is same as

    if (checkDate (year))
    
  2. checkDate() is validating a year from your context, so it should be checkYear() or validateYear().


Though I like to use flag (this is my opinion) for better understanding and code-refactoring.

public static int scanYear() {
    boolean isYearValid = false;        
    int year = 0;       
    do{
        int year = TextIO.getInt();
        if (checkYear(year))
            isYearValid = true;
        else
            TextIO.put("year not in the allowed range");
    }while (!isYearValid);
    return year;
}
\$\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.