Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am having difficulty deciding how to implement an exception handling strategy.

I am using an observer pattern to allow "plugin" programmers to subscribe to Messages. These subscribers generally log a unique error (and do some other stuff) under an exceptional circumstance during handling the message. The code snippet below is an example of what a common implementation (and its interface) looks like:

// API interface
interface IMessageListener {
   void onMessage(Message m);
}

class StuffMessageListener implements IMessageListener {
    ...

    @Override void onMessage(Message m) {

        try {
            Stuff s = stuffDAO.getStuff(..); // throws SQLException

            ...

            m.reply(true);
        } catch (SQLException e) {
            // if an exception happens, log it, and reply to the message
            log.error(A_UNIQUE_ERROR_CODE, e);
            m.reply(false);
        }
    }
}

What irks me is that the catch block has become very redundant code, copied throughout 95% of the ~50 existing listeners. Can we come up with something better?

A good solution should:

  • be easy to use/understand for an amateur programmer
  • keep the programmer aware of the existence/potential of exceptions
  • minimize the chance of a programmer accidentally "swallowing" an exception
  • reduce the amount of "boilerplate" code

    • UPDATE:

    Thanks everyone for your answers. I wanted to mention that there is a subtle requirement needed for the solution. The UNIQUE_ERROR_CODE should be unique across all declarations of the listeners. That way, I could gain statistics based upon where the error was caught across all listeners.

    If the UNIQUE_ERROR_CODE is buried underneath an abstract class, or is used outside onMessage(..), I will lose the needed "uniqueness."

    share|improve this question
    What does m.reply(boolean) do? – Nick ODell Aug 1 '12 at 17:40
    It signals to the sender of the message that the handling was (un)successful. – Beefyhalo Aug 1 '12 at 17:42
    Why not make it assume by default that it was unsuccessful? – Nick ODell Aug 1 '12 at 17:44
    1  
    this should belong on code review. – Daniel A. White Aug 1 '12 at 17:46
    Thx for the suggestion @NickODell. I could use that strategy and add a suppressReply() method, but I'm not sure how much I like that.. The reason for this new method is because in some cases, the message should not be replied to. – Beefyhalo Aug 1 '12 at 17:47

    migrated from stackoverflow.com Aug 1 '12 at 18:06

    3 Answers

    up vote 8 down vote accepted

    Allow throwing exceptions

    Do you really think your user is capable of handling all the exceptions?

    interface IMessageListener {
   void onMessage(Message m) throws Exception;
}

    Of course if some plugin wants to handle the exception, nothing prevents he/she of using try/catch. But no one is forced to do so and if, according to your knowledge, the catch block is almost always the same, put it outside in the code calling onMessage().

    Give your user a simplified adapter

    abstract class ThrowingMessageListenerAdapter implements IMessageListener {

    @Override
    public void onMessage(Message m) {
        try {
            doOnMessage(m);
        } catch (SQLException e) {
            // if an exception happens, log it, and reply to the message
            log.error(A_UNIQUE_ERROR_CODE, e);
            m.reply(false);
        }
    }

    protected abstract void doOnMessage(Message m) throws Exception;

}
    share|improve this answer
    I really like this answer. However, there is a fixed requirement that the user must not throw a exception in the onMessage() (like JMS). Please check my update to know why the adapter solution will not preserve unique error codes. – Beefyhalo Aug 1 '12 at 20:50
    up vote 5 down vote

    This looks like a good place to use an abstract class instead of an interface:

    abstract class MessageListener{
    void onMessage(Message m){
        try{ onMessageLogic(m); }
        catch(Exception e){ ... }
    }
    abstract void onMessageLogic(Message m) throws Exception;
}

class StuffMessageListener extends MessageListener{
    onMessageLogic(Message m) throws Exception{  
        /* possibly exception-raising code here */  
    }
}
    share|improve this answer
    up vote 0 down vote

    best thing would be to

    implement getStuff method with Message m as a parameter to it. This way you can catch the exception inside your method which set

    m.reply(true) or m.reply(false)

    inside your code itself making it easier for others to use it.

    share|improve this answer
    Side effects should be avoided, not encouraged. – Wug Aug 1 '12 at 17:55

    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.