10

I've written a function that asks a user for input until user enters a positive integer (a natural number). Somebody said I shouldn't throw and catch exceptions in my function and should let the caller of my function handle them.

I wonder what other developers think about this. I'm also probably misusing exceptions in the function. Here's the code in Java:

private static int sideInput()
{
    int side = 0;
    String input;
    Scanner scanner = new Scanner(System.in);

    do {
        System.out.print("Side length: ");
        input = scanner.nextLine();
        try {
            side = Integer.parseInt(input);
            if (side <= 0) {
                // probably a misuse of exceptions
                throw new NumberFormatException();
            }
        }
        catch (NumberFormatException numFormExc) {
            System.out.println("Invalid input. Enter a natural number.");
        }
    } while (side <= 0);

    return side;
}

I'm interested in two things:

  1. Should I let the caller worry about exceptions? The point of the function is that it nags the user until the user enters a natural number. Is the point of the function bad? I'm not talking about UI (user not being able to get out of the loop without proper input), but about looped input with exceptions handled.
  2. Would you say the throw statement (in this case) is a misuse of exceptions? I could easily create a flag for checking validity of the number and output the warning message based on that flag. But that would add more lines to the code and I think it's perfectly readable as it is.

The thing is I often write a separate input function. If user has to input a number multiple times, I create a separate function for input that handles all formatting exceptions and limitations.

1
  • Highly depends on language. Some languages use exceptions more freely than others. Nov 22, 2011 at 21:17

6 Answers 6

11

The point of an exceptions is that it allows a method to tell that caller that entered a state where it could not continue normally, without forcing you to embed an error code in the return value.

In your case, your method knows exactly what to do when the input is not greater than 0. The only reason you are saving lines here is because you happen to be throwing the same exception you would get if the input was not a number. However, the exception you are throwing does not properly represent why your code does not like the input. If someone else were to come along and see this code, they would have to spend extra time trying to see exactly how things are working.

2
  • Yeah, that's why I thought it was a misuse. So, ignoring that throw statement, you agree that it's OK to handle exceptions in such loop inside a function instead of letting the caller catch them? The catch statement is there because of Integer.parseInt (again, ignoring the throw).
    – usr
    Nov 22, 2011 at 20:39
  • 1
    @usr: That answer depends much more on how the rest of the system is meant to work together. Exceptions need to be handled at some point. There are a few different ways you could organize it and this is one of them. Which is best depends on other information we don't have here. Nov 22, 2011 at 20:51
5

This is a bad use of exceptions. To begin with, a non-positive number is not a format exception.

Why use exceptions at all? If you know what input is not allowed, just don't break out of the loop until you get valid input from the user, something like the following:

while (true)
{
   // Get user input.
   String input = scanner.nextLine();

   try
   {
      side = Integer.parseInt(input);

      break;
   }
   catch (NumberFormatException ex)
   {
      // Inform user of invalid input.
      System.out.println("Invalid input. Enter a natural number.");
   }
}
5
  • Integer.intParse throws a format exceptions, so using the catch statement is perfectly valid. I mainly want to know if it's OK to use catch statement in a loop in a function or should I let the caller of the function handle format exception.
    – usr
    Nov 22, 2011 at 20:47
  • Integer.parseInt() throws a NumberFormatException if it can't parse the provided string argument. There is no need for you (and you won't be able) to throw the exception yourself.
    – Bernard
    Nov 22, 2011 at 20:51
  • I have edited the code example to be more explicit.
    – Bernard
    Nov 22, 2011 at 20:56
  • "and you won't be able) to throw the exception yourself" - what do you mean there? Nov 22, 2011 at 21:24
  • @Michael Borgwardt: I mean that since the Integer.parseInt() method will throw the exception for you if it occurs, you won't be able to throw it yourself afterward since it has already been thrown.
    – Bernard
    Nov 22, 2011 at 21:58
1

Only catch exceptions if you intend to do something that is relevant to the current method call; i.e cleanup, failure logic etc. In this case, the catch is merely sending a message to the console, it is not relevant to sideInput method, so therefore it can be handled further up the call chain/stack.

One can get rid of the try/catch here and simply document the method call:

//Throws NumberFormatException if read input is less than 0
private static int sideInput()

One still needs to handle that exception further up the call chain/stack!

1
  • 1
    The message is there just for a better UI. The point of the function is that it nags the user for input until she/he enters a valid input. This can't be done without try-catch blocks. My question was if the point itself is valid. I think it is, but someone told me I should remove the try-catch blocks and let the caller handle this particular exception. But then the function won't work as intended.
    – usr
    Nov 25, 2011 at 13:11
1

You should not both throw and catch the same exception in a method, I even think the the catch block will catch the same exception you are throwing, so you are not really throwing it.

If parseInt was successful, then it's not a NumberFormatException.

if side is less than zero, you should throw a NegativeSideLengthException;

Create a custom/bussiness Exception named NegativeSideLengthException

public class NegativeSideLengthException extends Exception
{


    public NegativeSideLengthException(Integer i)
    {
        super("Invalid negative side length "+i);        
    }

}

Then sideInput throws NegativeSideLengthException

private static int sideInput() throws NegativeSideLengthException
{
    int side = 0;
    String input;
    Scanner scanner = new Scanner(System.in);

    do {
        System.out.print("Side length: ");
        input = scanner.nextLine();
        try {
            side = Integer.parseInt(input);
            if (side <= 0) {
                throw new NegativeSideLengthException(side);
            }
        }
        catch (NumberFormatException numFormExc) {
            System.out.println("Invalid input. Enter a natural number.");
        }
    } while (side <= 0);

    return side;
}

You can even ( if you want ) add another catch block to catch NegativeSideLengthException and not have the method throw it.

do {
    System.out.print("Side length: ");
    input = scanner.nextLine();
    try {
        side = Integer.parseInt(input);
        if (side <= 0) {
            throw new NegativeSideLengthException(side);
        }
    }
    catch (NumberFormatException numFormExc) {
        System.out.println("Invalid input. Enter a natural number.");
    } catch (NegativeSideLengthException e){
        System.out.println("Invalid input. Enter a non-negative number.");
    }
} while (side <= 0);

Flags are not a good way to handle exceptions.

1

There’s the general case, and your specific case.

A general rule “you mustn’t throw and catch in the same method” is nonsense. If your function detects an exceptional situation then it throws an exception. If it catches exceptions (and handles them properly) you catch exceptions. If your function does both you do both. It is slightly unusual, but that doesn’t matter.

Now in your particular case: Non-positive numbers are not an exceptional situation. Therefore you shouldn’t throw an exception. And you are throwing the wrong exception. Better to have two different error messages for situations where the parser throws an exception, and for non-positive input.

And your code is quite complex. What would happen if you didn’t initialise “side” or initialised it to +1? Or you changed the requirement to side >= 0?

-1

Exceptions are pretty nightmarish things, they bring more complexities than they solve.

Firstly, if you don't catch your exceptions, the caller can only do on error resume next, that is, after one week, even you won't know what your function can throw, and what to do with it:

{
    ...
}
catch(OutOfMemory, CorruptedMemory, BadData, DanglingPointers, UnfinishedCommit)
{
    Console.WriteLine("Nothing to see here, move on.");
    Console.WriteLine("The app is very stable, see, no crashing!");
}

Basically, if you catch them, you have to have a very good understanding about contracts, and exception guarantees. That rarely happens in a real world. And also your code will be hard to read.

Also, the funny thing is that if you really have any chance of handling exceptions you need real RAII language, which is sort of humorous, since Java and .NET is all about exceptions...

Repeating this one more time, but...:

Link

Link

http://www.joelonsoftware.com/items/2003/10/13.html

9
  • 5
    -1 for posting a borderline rant, full of not quite correct -or at least context/language dependent - statements, instead of answering the OP's actual question. Nov 22, 2011 at 22:20
  • @PéterTörök: "Give a man a fish and you feed him for a day. Teach a man to fish and you feed him for a lifetime." There are very serious problems behind exceptions, and people should know them -1000/+1, I don't really care.
    – Coder
    Nov 23, 2011 at 16:59
  • 1
    Feel free to believe you can write correct code easier without than with exceptions. Just don't state your views and beliefs as facts (even if Joel is of the same opinion, it is still an opinion, not a fact), and don't post irrelevant answers on SE. Nov 23, 2011 at 18:46
  • @PéterTörök: It's not an opinion, it's a fact, every time you use a component that throws an exception internally, you have to crawl the whole hierarchy and check every single line of code to know what to catch, and if the components offer strong guarantee, and everything is rolled back, or if that catch is just a false sense of security. Heck, you don't even know all the exceptions std::string throws, you can look up the specs, but you'll never find. Things like : throw(thisexception, thatexception) are downright wrong and should never be used, because otherwise you'll get unexpectedexcept.
    – Coder
    Nov 23, 2011 at 22:02
  • 2
    OK, so how about code not using exceptions? Gee, you need to crawl through the code to check every single line to see whether return values are handled properly or ignored. And when a return value is ignored, noone notices until maybe your app crashes a couple thousand lines later. Exceptions at least force you to take notice. Yes, you can swallow them - but only with an explicit catch. While an ignored return value is unsearchable - it is identifiable only via line-by-line code review. Last but not least, constructors have no return values, this is the foremost reason to use exceptions. Nov 24, 2011 at 8:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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