5
\$\begingroup\$

I have a class that has 5 similar methods, they just relay input parameters and return a result - best practice would be to skip the class, but that's not what I'm asking about :) This is a method from the class:

public String deleteItem(Integer itemId,String username, String kpNumber) {
        try {
            return getItemServiceFacade().deleteItem(itemId, username, kpNumber);
        } catch (RemoteException re) {
            String message = "5:unexpected error:" + re.toString();
            logger.error(message);
            return message;
        } catch (CoreException ce) {
            String message = "5:unexpected error:" + ce.toString();
            logger.error(message);
            return message;
        }
    }

Now, since all the methods are similar, each has the same catch block, each does the same things... so I'd like to rewrite this but I'm unsure how far should I go in "atomizing" the methods. One way is what I consider the "pure" way, so the new code looks like this:

public String deleteItem(Integer itemId,String username, String kpNumber) {
        try {
            return getItemServiceFacade().deleteItem(itemId, username, kpNumber);
        } catch (RemoteException re) {
            logException(re);
            return getMessage(re);
        } catch (CoreException ce) {
            logException(ce);
            return getMessage(re);
        }
    }


private String getMessage(Exception exception){
    return "5:unexpected error:" + exception.toString();
}

private void logException(Exception exception){
    logger.error(getMessage(exception));
}

but I'm also left wondering if I should combine these two methods into one method, like this

public String handleException (Exception e){
    String message = "5:unexpected error:" + exception.toString();
    logger.error(message);
    return message;
}

so what I guess I'm trying to ask is how far should you go in separation of concerns when you have clear and simple cases like this one.

Edit: It seems I have not managed to make myself clear, so I'll try again :) In this case simple case, is it considered better to write one method:

public String handleException (Exception e){
    String message = "5:unexpected error:" + exception.toString();
    logger.error(message);
    return message;
}

or should I have two:

private String getMessage(Exception exception){
    return "5:unexpected error:" + exception.toString();
}

private void logException(Exception exception){
    logger.error(getMessage(exception));
}
\$\endgroup\$
2
  • \$\begingroup\$ In what way are those methods different? \$\endgroup\$ Jun 19 '12 at 13:07
  • \$\begingroup\$ In this two lines public String deleteItem(Integer itemId,String username, String kpNumber) and return getItemServiceFacade().deleteItem(itemId, username, kpNumber); \$\endgroup\$
    – Andrija
    Jun 19 '12 at 13:28
3
\$\begingroup\$

One alternative, although I don't know if you think it's too complex, would be to apply a visitor pattern:

private interface Command {
    String apply(ItemServiceFacade facade) throws RemoteException, CoreException;
}

public String deleteItem(final Integer itemId, final String username, final String kpNumber) {
    return execute(new Command() {
        @Override
        public String apply(ItemServiceFacade facade) throws RemoteException, CoreException {
            return facade.deleteItem(itemId, username, kpNumber);
        }
    });
}

    // other methods...

private String execute(Command command) {
    try {
        return command.apply(getItemServiceFacade());
    } catch (RemoteException re) {
        String message = "5:unexpected error:" + re.toString();
        logger.error(message);
        return message;
    } catch (CoreException ce) {
        String message = "5:unexpected error:" + ce.toString();
        logger.error(message);
        return message;
    }
}

I used this pattern when faced with a similar though slightly more complicated situation (implementing optimistic locking for a few different operations).

\$\endgroup\$
1
  • \$\begingroup\$ This is a bit too complex, but the thing is, I was asking about optimizing the part in the Catch block, not making the method call generic :) Plus, I'm working on Java 1.4 :) \$\endgroup\$
    – Andrija
    Jun 19 '12 at 13:53
1
\$\begingroup\$

There are several things you might want to think about.

The method returns a string. I'm not sure what the facade might return, but to me a boolean or the id (-1 if the method invocation fails) would make more sense. Return the string created by the exceptions requires you not only to implement the try {} catch {} finally block for the checked exceptions, but also forces you to check for an error when the method has been invoked. Since that is what you are returning to the system.

I guess i would think of something like

public boolean deleteItem(Integer itemId,String username, String kpNumber) {
  try {
    getItemServiceFacade().deleteItem(itemId, username, kpNumber);
  } catch (Exception e) {
    // log it away in a descriptive way, you just pass the Exception instance to the method.
    return false;
  }
  return true;
}

in contrast to what kyck-ling did, which is a nice way as well. But in the end, as far as i can see, it can be simpler for those cases. But that's always the problem - we don't have any inside regarding the rest of the application.

\$\endgroup\$
2
  • \$\begingroup\$ The facade also returns a string, in the form of "1:11:2012" where the "1" means success and the rest is calculated data. Example of unsuccessful message would be "3:id does not exist!". But again, I'm not so much interested specifically how to optimize this method, but more generally, what's better practice, making a method do two things at once since it's really simple, or making a full separation of concerns while keeping in mind that the use case is simple and we assume nothing will change in the way the methods behave - more methods could be added but the method logic won't change. \$\endgroup\$
    – Andrija
    Jun 19 '12 at 14:23
  • \$\begingroup\$ Well, best practice is concise and clean code. You gain nothing by explicitly adding new try {} catch blocks because you handle the errors independently from Java offers. \$\endgroup\$ Jun 19 '12 at 14:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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