Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

share|improve this question
1  
This seems pretty specific to the behavior of the library/framework you are using. Could you provide some details on that? –  Rein Henrichs Apr 3 '13 at 16:24
1  
We have a particular user we refer to as "PICNIC" (problem in chair, not in computer) -- I suppose one way to handle a PICNICException is to show a popup asking the user to leave the computer and let someone else take over. –  p.s.w.g Apr 3 '13 at 17:07
    
@ReinHenrichs I'm not entirely sure how I can, the only pertinent library in this case is the JExcel library. I'm still quite new to this; what do you mean by 'framework'? –  Pureferret Apr 3 '13 at 17:10
    
@p.s.w.g it was just a silly exception name, and not indicative of my code, but I do see merit in your solution. –  Pureferret Apr 3 '13 at 17:14
1  
@Pureferret PICNICException is a class like any other, is it not?. PICNICException e = new PICNICException(); e.setStuff("hi"); throw e; and then on the catching end: catch(PICNICException e){ showMessage(e.getStuff());}. You can add any kind of first-class (not a string that you parse) custom data to the exception, throw it and retrieve it on the catching end. –  Esailija Apr 3 '13 at 21:10

3 Answers 3

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.

share|improve this answer
    
That sounds like I'm in the clear, it was just the whole static class with a list or errors, etc. that felt fishy. Thanks! –  Pureferret Apr 3 '13 at 17:13
    
@Pureferret that is very fishy :P, see my comment on your question –  Esailija Apr 3 '13 at 17:27
    
Agreed - I skipped over the static class with a list of errors and focussed on just the exception part. –  Michael Apr 4 '13 at 8:41

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.
share|improve this answer
    
On the first part, I have that covered, but I wanted to go for a silly name here instead. On the second: it is better to either generate the user-visible message at the point where you display it Surely the message is specific to what caused the exception? It might name which sheet was empty, something not easily divined at the point of catching, and put it in the exception object itself Again, I might want to customize the message at the point of throwing, which I don't know how to do. –  Pureferret Apr 3 '13 at 17:19
1  
you mean like `throw new PICNICException("My Specific Error Text"); ? Exceptions are just objects, you can put whatever members you want in them. –  Useless Apr 3 '13 at 17:46
1  
@Pureferret: As Useless already indicated in his comment, an Exception is also an object, so it can have additional attributes and methods. The correct solution would be to use throw new SheetEmptyException("sheet name") ... catch(SheetEmptyException e) { showMessage("The sheet'" + e.getSheetName() + "' is empty") }. This way the relevant information is carried from the throw point to the catch point, but the throw point does not have to know how it gets presented to the user. –  Bart van Ingen Schenau Apr 4 '13 at 12:30

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?
share|improve this answer

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.