Am I handling my exceptions in a sensible manner?

Question

I'm still restructuring the code I have been given to update on my current work project, and I've come to the point where I'm looking at how the code handles an 'exceptional' input, that doesn't conform to the business logic.

In essence, the code stitches together various different excel spreadsheets, tarts them up and lets the user 'download' them to their workspace for later analysis.

This is what I have set in the code:
If the code ends up (anywhere) trying to, for instance, import an empty spreadsheet into the final work book (something that should never happen, under the business logic) then write out to a LinkedList variable (ErrorList) in a static class (MyVariables) like so, the error:

MyVariables.ErrorList.add("My Specific Error Text");

Then I throw an exception like so:

throw new PICNICException();

Then in the frame that called the trail of methods, the exception is finally caught and I can write, something like this:

catch(PICNICException Pe){
    JOption.showMessageDialog(This, MyVariables.ErrorList.getLast(), "Title Relevant to what Code is Doing", JOption.ERROR_DIALOG)
   Pe.printStackTrace();
}

I have feel I have to do this as, I have no way to pop up the Message Dialog (which is the desired behaviour on erroring) from within the method that's thrown this exception. Without completely refactoring the entire code base, and making a singleton that can be accessed anywhere to handle these message dialogues, I don't see any other way of doing this.

I can't help but feel this is a gigantic hack, and there is a better method for doing this.

Am I handling my exceptions in the best possible manner, given my situation? What would the best practice be?

Answer 1

Throwing exceptions is not bad, throwing exceptions for a non exceptional situation is considered poor but this sounds like you're throwing the exception for an exceptional reason so I think it's fine.

Things I'd keep in mind though:

1. Throw a specific exception
2. Log information before the exception is thrown
3. Call out in the documentation that this method can throw the exception

Other than that, we shouldn't try to avoid exceptions in all cases - they do come in very useful, we just shouldn't be using them for standard control flow.

Answer 2

Your use of exceptions seems entirely appropriate here.

The code encounters a situation that should never occur and that it can't reasonably handle locally. That sounds like the kind of situation that exceptions were meant for.

However, I have also some points of critique:

1. Your exception name does not seem to indicate what kind of problem the code encountered (I would have expected something along the lines of EmptyWorkbookException). This means that it will later be harder to implement different error handling for different problems (for example, one error requires a 'Critical' messagebox, while another requires a 'Warning' messagebox)
2. Instead of using the separate variable ErrorList, it is better to either generate the user-visible message at the point where you display it (and thus also know what language to use), or to put it in the exception object itself. The first option is generally preferred for end-user visible text, so you don't have to bother with localisation all over the place. The second option should be used to carry information in general from the throw site to the catch site.

Answer 3

In this case I would remove the ErrorList and include some kind of key long with the exception e.g. :

throw new PICINCException(ErrorKey.EMPTY_SPREADSHEET);

The error key then maps into a set of messages with specific error messages for your GUI. This has some advantages over the current solution:

1. All your error messages are in a single place which makes them easy to update.
2. If you're ever likely to localise the app (it sounds unlikely but you would be surprised) it would be easier to do user specific locales.
3. With proper naming it should be obvious to anyone reading the code what is going on with reduced exposure ("I see an ErrorKey enum so that must map to the error messages").
4. It reduces the boiler-plate around adding to the ErrorList then throwing the exception, what happens if I forget to add to the ErrorList then I throw an Exception?