5

I inherited a java application that processes requests and throws an exception if it determines a request should be cancelled. Exceptions have been convenient for the previous developer because they are an easy way to exit out of a tree of logic that no longer applies (yes a goto) and it prints a stack trace to the log which is good info to have. It seems to be the consenus on forums and blogs that exceptions should not be used for flow control and over half of the requests processed are cancelled, so they're definitely not exceptional situations. One argument is performance, which doesn't apply because our exception-ful code has run fast enough for years. Another argument is that the goto-ness of it is not clear, which I agree with. My question is: What is the alternative. The only thing I can think of is to have every method return true if processing should continue or false if it shouldn't. This seems like it would hugely bloat my code changing this:

checkSomething(); checkSomethingElse();

into this:

boolean continueProcessing = false; continueProcessing = checkSomething(); if (!continueProcessing) { return false; } continueProcessing = checkSomethingElse(); if (!continueProcessing) { return false; }

And then what if the method is supposed to return something? Any guidance would be great. I'd really like to observe whatever "best practices" are available.

Update:

Another bit I probably should have mentioned in the first place is that a request is cancelled more than 50% of the time and does not mean something didn't go right, it means the request wasn't needed after all.

Improve this question
3
  • I think checked exceptions describe intent perfectly (as opposed to GOTOs). I don't see a problem with this. Make sure to define your own exceptions, with enough concrete information to handle your exceptional cases (multiple catch blocks are fine for this). Commented Apr 26, 2011 at 23:57
  • 1
    There's nothing wrong with goto when they're used appropriately. The only thing is, most of the time, they're not used appropriately. Commented Apr 27, 2011 at 0:43
  • 2
    Don't change anything for the sake of change. It works, and works good -- let it be. Abstract ideas are good, but working code is better. Commented Apr 27, 2011 at 1:16

5 Answers 5

Reset to default
6

In your scenario, the alternatives are throwing an exception and returning a result.

Which is best (and which is a "best practice"1) depends on whether the failure of the "check..." methods should be classed as normal or exceptional. To some degree this is a judgement call that you have to make. In making that call there are a couple of things to bear in mind:

  • Creating + throwing + catching an exception can be roughly 3 orders of magnitude2 SLOWER than testing a boolean result.

  • The big problem with returning status codes is that it is easy to forget to test them.

  • A second problem with status codes is that they often have to be passed through many layers, where the codes are not the concern of the intermediate layers. Relying on the intermediate layers to know to pass the code is a Separation of Concerns issue.

In summary, "best practice"1 in Java3 is to not use exceptions to implement normal flow control, but it is up to you to decide where the borderline between normal and exceptional is.

1 - So called "best practice" is really just opinion. No Best Practices makes the case that we should not use the phrase.
2 - This depends on the Java implementation.
3 - But not necessarily in other programming languages.

Without seeing the real code / entire context of your application, we can't advise you on the best way to do this. Your "fails 50% of the time" amendment to the question is only part of the context.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Previous developer knew his business well. Instead of spagetti code -- one clean throw. If he managed to provide for cleaning of the changed state, he's a God Who Shines.
I updated the question above to reflect that a request is cancelled more than 50% of the time and that cancellation just means that a request was not needed after all, not that there was a problem. Again performance is not an issue. This app runs on a server at night and is plenty fast enough.
4

See How slow are Java exceptions? for a great discussion on this topic.

Improve this answer

Comments

1

tl;dr

Separation of Concerns, and IMO you should do this:

continue = checkSomething() && checkSomethingElse();

or perhaps there are other design problems in the code.

What's the concern of the function -- as you want to define it (this can be a subjective question)? If the error fits into the function's concern, then don't throw an exception. If you are confused about whether or not the error fits into the concern, then perhaps the function is trying to accomplish too many concerns on its own (bad design).

Error control options:

  1. don't report error. It either is handled directly by function or doesn't matter enough
  2. return value is
    1. null instead of an object
    2. the error information (perhaps even a different data type than the object returned on success).
  3. an argument passed in will be used to store error data.
  4. trigger an event
  5. call a closure passed to function if an error occurs.
  6. throw an exception. (I'm arguing this should usually only be done if it's not a part of the arbitrarily defined purpose of the function)

If the purpose of the code is to check some state, then knowing that the state is false is directly the point of the function. Don't throw an exception, but return false.

That's what it looks like you are wanting. You have process X which is running checkers Y and Z. Control flow for process X (or any calling process) is not the same concern as checking states of a system.

Improve this answer

Comments

-1

How about

if (!checkSomething()) return false; if (!checkSomethingElse()) return false;

No need for the flag if you exit early.

Improve this answer

1 Comment

@ThomasS: Guard clauses are cleaner than any alternative i can think of. I'd personally find it a bit harder to deal with a bunch of nested conditionals, which would be the "acceptable" alternative. If checkSomething etc were named something better, like somethingIsBroken, and returned true if it's broken of course, this would make more sense. But i still maintain that the idea is sound. Certainly less hairy than using exceptions for normal flow. That'd be far worse than a goto; it's more like COMEFROM from INTERCAL, a language designed to be hard to use.
-1 
int error = 0; do {//try-alt error = operation_1(); if(error > 0){break;} error = operation_2(); if(error > 0){break;} error = operation_3(); if(error > 0){break;} }while(false); switch(error) //catch-alt { case 1: handle(1); break; case 2: handle(2); break; case 3: handle(3); break; }
Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.