Checking for a cache hit

Question

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;
    }
}

Answer 1

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
            && 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

Answer 2

This is not really very different from what you had, but here goes.

private boolean useCachedReportBean(BeanReport cachedReportBean, String strRequestedReportID) 
    throws ExceptionInvalidReportRequest 
{
    if (isEmpty(strRequestedReportID)) {
        if (cachedReportBean == null) {
            throw new ExceptionInvalidReportRequest(
                    "The request ID is invalid and no cached report exists.");
        } else {
            return true;
        }
    }
    else if (cachedReportBean == null) {
        return false;
    }
    else {
        return strRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID());
    }
}

I think the if/else if structure provides some symmetry with the 3 primary conditions (no requested report id, no cached report bean, both present), and more clearly demonstrates that it can only be one of those 3.

Note: I am also assuming the existence of a string utility function isEmpty.

boolean isEmpty(String s) {...}

Answer 3

Unified representation of ONE semantic

If "strRequestedReportID == null" and "".equals(strRequestedReportID) have the same semantic this should be unified as early as possible in previous code (maybe where the request occured) so the provided method and maybe all other methods need only to handle one case. I suggest to use an empty String that is the neutral element for String operations.

Responsibilities

Throwing an ExceptionInvalidReportRequest is not the responsibility of this method. Why ask for the usage of a cached ReportBean when it is not even passed in as a parameter? This should be handled before this method is called.

Multiple return statements

Try to avoid multiple returns. Breaking the control flow can hinder you to apply refactorings like extract method. So you should be sure that your code does not violate SRP or it will never change again when using multiple return statements, break or continue.

Code in guessed environment

As only one method for review is provided I can only guess how the environment looks like. So I made up a setup to include the method with the semantic I analyzed.

The point is: The method as provided is effectively one line. For me it makes perfect sense that "A cached report bean should only be used if it is the requested one". All other stuff I think does not belong to this method. Therefore it must be located in the environment I made up.

private BeanReport cachedReportBean;


private BeanReport getBeanReport(String strRequestedReportID) throws ExceptionInvalidReportRequest {
    
    // this should be done as early as possible 
    String normalizedRequestedReportID = strRequestedReportID == null ? "": strRequestedReportID;
    
    BeanReport reportBean = null;
    
    if ("".equals(normalizedRequestedReportID) && cachedReportBean == null) {
        throw new ExceptionInvalidReportRequest(
                "The request ID is invalid and no cached report exists.");
    }
    
    if (cachedReportBean != null && useCachedReportBean(cachedReportBean, normalizedRequestedReportID)) {
        ...
        reportBean = cachedReportBean;
        ...
    } else {
        ...
        reportBean = loadReportBean(normalizedRequestedReportID);
        ...
    }
    
    ...
    
    return reportBean;
}

private boolean useCachedReportBean(BeanReport cachedReportBean, String normalizedRequestedReportID) {
    return normalizedRequestedReportID.equalsIgnoreCase(cachedReportBean.getConfigBean().getReportID())
}