Refactoring : Logical Operators Instead of Conditional Flow

scuilion

I’ve recently been reading Working Effectively With Legacy Code by Feathers (an oldie, but a goodies). The book is good for enumerating techniques of refactoring but more importantly, it is a reminder to be more observant when looking through old code, and that is exactly what happened to me this weekend. I was trying to familiarize myself with how our application used OSGi inside of a web container when I came across the following code. It looks as if it had been written in stages. Something akin to: “Try it this way. Oh, that worked here but not there. Add another thing…oh, and there is another use case.” At any point, when the programmer realized they needed to add another test case, they should have use the Sprout technique. After all, it doesn’t take much to create a method and put the code down a few lines. There are several other concerning thing about the code. I’ll enumerate them below and rewrite the method a couple of times.

public boolean Startup() {
    boolean started = true;
    // first try
    String osgiServletURL = URLUtils.getUrl() + BRIDGE_URL;
    started = callBridgeServlet(osgiServletURL);

    // recovery part 1  
    if (!started) {
        InetAddress thisIp = null;
        try {
            thisIp = InetAddress.getLocalHost();
        } catch (UnknownHostException e) {
            started = false;
        }
        if (thisIp != null && thisIp.getHostAddress() != null) {
           String thisIpAddress = thisIp.getHostAddress().toString();
           osgiServletURL = createOSGIServletURL(thisIpAddress);
           started = callBridgeServlet(osgiServletURL);
        } else
           started = false;
    }
    // recovery part 2 
    if (!started) {
        osgiServletURL = createOSGIServletURL(LOCALHOST_NAME);
        started = callBridgeServlet(osgiServletURL);
    }

    return started;
}
  • Boolean success variables: The started variable is hard to keep track of especially with all those conditional branches. (Should this variable be initialized to true‽)
  • Nested conditional statements: Not only that, but there are missing brackets around the last else statement, making it harder to read.
  • The method is making three attempts to connect to the servlet and in one case it’s checking for an exception but not in the others, making the code asymmetrical?
  • A variable that changes meaning: Notice how osgiServletURL holds values that mean different things as it goes through the method. At one point it’s a URL at another it’s an IP address.

Let’s do a little clean up in our first pass. Eliminate unnecessary comments (the code will be obvious when we get done), pull out method-wide variables, and if you have to use a boolean success variable make sure you initialize it to the false, otherwise you’ll constantly be setting the value and you’re likely to miss a case.

public boolean startup() {
    boolean started = false;
    String osgiServletURL = URLUtils.getUrl()+BRIDGE_URL;
    started = callBridgeServlet(osgiServletURL);

    if (!started) {
        InetAddress thisIp = null;
        try {
            thisIp = InetAddress.getLocalHost();
            if (thisIp != null && thisIp.getHostAddress() != null) {
                String thisIpAddress = thisIp.getHostAddress().toString();
                osgiServletURL = createOSGIServletURL(thisIpAddress);
                started = callBridgeServlet(osgiServletURL);
            }

        } catch (UnknownHostException e) {
        }

    }
    if (!started) {
        osgiServletURL = createOSGIServletURL(LOCALHOST_NAME);
        started = callBridgeServlet(osgiServletURL);
    }

    return started;
}

Looking a little bit better. Let’s create some sprout method to clean up that ugly looking nested exception(canConnectWithUrl Line 2-4). We’re also going to create two more sprout methods so that they will have similar names(canConnectWithIpLine 7-17 and canConnectWithLocalHostNameLine 21-22). Ultimately, just a readability thing.

public boolean startup() {
    boolean started = false;

    started = canConnectWithUrl();
    if (!started) {
        started = canConnectWithIp();
    }else if (!started) {
        started = canConnectWithLocalHostName();
    }

    return started;
}

And, finally, to get rid of the if statements and the boolean success variable, let’s use logical operators:

public boolean startup() {
    return canConnectWithUrl() 
        || canConnectWithIp() 
        || canConnectWithLocalHostName();
}

Java will call each method in turn and return as soon as the first one is true. So we have code that reads like this: “Can it connect using the url or can it connect with an ip address or can it connect with the localhost name”. If the code were written this way yesterday, I would have been able to read it, understand it, and move on to the next step within a minute. (Not to mention that the refactored code will be easier to test.)

This is not a complex case study, more similar to the type of pointers that I would give during a code review. Hopefully it will stand as a reminder that taking a second look at code can go a long way in readability and maintainability.

Leave a comment