Hi all. First post... be gentle!
I was performing some desperately needed refactoring in one of our larger and more spaghettified web-based systems. More often than not the only error handling in the system is to just catch 'Exception' at the controller level, rethrow it, and let the application container return an HTTP 500.
In a specific example of this, there is a bit of logic that says a photo for an item can only be modified either if the item's sale date is more than a pre-set cut-off period away, or by an administrator. The code was seemingly rather simple, and didn't seem to need to throw a generic exception...
String PHOTO_FLAG = null;
try {
PHOTO_FLAG = setPhotoFlag(session, item);
} catch (Exception e) {
logger.error("An error has occurred setting the Photo flag.", e);
throw new SystemRuntimeException(e);
}
returnModelAndView.addObject(PHOTOSTATUS, PHOTO_FLAG);
Why does setPhotoFlag() throw Exception, and why dont we bother trying to recover from this? I dug a little deeper...
private String setPhotoFlag(HttpSession session, Item item) throws Exception {
UserDetailsForm userDetailsForm = (UserDetailsForm) session.getAttribute(SystemWebConstants.USERPROFILE);
boolean allowUpdate = candidateManager.doAllowUpdate(item.getSaleDate(), userDetailsForm.getImper());
if (!allowUpdate) {
return "YES";
} else {
return "NO";
}
}
Ok, even disregarding the many WTFs here, including the fact that a method named 'setSomething' changes no state whatsoever, why does it even need to throw an exception at all, let alone Exception? Following the trail I found this...
public boolean doAllowUpdate(Date date, String impersonatingUserFlag) throws Exception {
int cutOffLimit = Integer.valueOf(resourceDataCache.getResourceProps().get(SystemConstants.PHOTO_CUT_LIMIT_PROPERTY));
return SystemGeneralUtils.addOrSubstractDaysFromDate(date, cutOffLimit).after(new Date()) || isAdminUser(impersonatingUserFlag);
}</code>
We're way past the controller level now... what kind of horrific, unrecoverable error condition could justify throwing Exception all the way up to the app server...
private boolean isAdminUser(String impersonatingUserFlag) throws Exception {
return ((null != impersonatingUserFlag) && SystemConstants.STR_YES.equals(impersonatingUserFlag));
}
WTF?! So not only does the method signature throw a generic exception, it throws that exception despite the fact that there aren't any checked exceptions in the code! Delightful. What exactly was the point of all this?
I suppose TRWTF is that for this extremely simple bit of logic there are 24 lines of code and 4 method calls.