Checking for a cache hit Please review and recommend improvements.  It looks horrible to me, difficult to read, too lengthy, and just plain old inelegant.  But I don't see the potential improvements.

private boolean useCachedReportBean(BeanReport cachedReportBean, String strRequestedReportID) 
    throws ExceptionInvalidReportRequest 
{
    /*  There may or may not be a cached report 
     * bean, and their may or may not be a requested
     * report ID.  If both exist, and they match, use 
     * the cached report bean.  If only one of 
     * the two exist, use it.   Finally if neither exist, 
     * throw an Exception.  
     */
    if (strRequestedReportID == null || "".equals(strRequestedReportID)) {
        if (cachedReportBean == null) {
            throw new ExceptionInvalidReportRequest(
                    "The request ID is invalid and no cached report exists.");
        } else {
            /* There was no requested ID but there is a cached bean.  
             * Returning true causes the cache to be used. */
            return true;
        }
    }
    /* If we are here, we know that there is a requested ID. 
     * If there is no cached bean, use the request id. */
    if (cachedReportBean == null) {
        return false;
    }
    /* There is a requested ID and cached bean.  If they match, use the cached bean. */ 
    if (strRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())) {
        return true;
    } else {
        return false;
    }
}
Rather than nesting if-else blocks it sometimes helps to cut the problem into independent slices and deal with them one by one, starting with the fairly exceptional ones:

private boolean useCachedReportBeanRefact(BeanReport cachedReportBean,
        String strRequestedReportID) throws ExceptionInvalidReportRequest {
    // validate input arguments
    if (cachedReportBean == null
            &amp;&amp; StringUtils.isBlank(strRequestedReportID)) {
        throw new ExceptionInvalidReportRequest(
                "The request ID is invalid and no cached report exists.");
    }

    // corner case #1
    if (cachedReportBean == null) {
        return false;
    }

    // corner case #2
    if (StringUtils.isBlank(strRequestedReportID)) {
        return true;
    }

    // after all corner cases are dealt with, just go ahead and compare the id
    return strRequestedReportID.equalsIgnoreCase(cachedReportBean
            .getConfigBean().getReportID());
}


I think the real problem is that this function does too much and is poorly named (it doesn't really use the cached report, it only checks if... a bean, if not null, has a config object whose ID is same as the other input parameter... on the condition that this other parameter is not blank either... I think?). It's not immediately obvious where it would be used and what the returned boolean would mean, it doesn't seem to translate to any real-life concept.

Making ExceptionInvalidReportRequest a checked exception feels like a bad idea, too (difficult to say without knowing the context); wouldn't unchecked IllegalArgumentException do?



Working code including unit tests: http://pastebin.com/pCXQXLee