Channels ▼
RSS

JVM Languages

Refactoring Deeply Nested Code


Let's look back at out original example code in Listing Two. What can we gain by flattening this code? Listing Seven contains the code with the deepest level collapsed.

Listing Seven

int dispatchTrigger(Trigger trigger, 
                    Configuration configuration,
                    Boolean isFallback) 
{
    if (isFallBack && !trigger.clear()) {
        if (configuration.getSetting(MUTEX_DISPATCH) 
                && trigger.getType() == ONE_SHOT 
                && trigger.atType() == PERIODIC) 
            trigger.fire();
        if (configuration.getSetting(MUTEX_DISPATCH) 
               && trigger.getType() == ONE_SHOT 
               && !(trigger.atType() == PERIODIC)) 
           trigger.registerPoll();
    } 

    if (!(configuration.getSetting(MUTEX_DISPATCH) 
               && trigger.getType() == ONE_SHOT 
               && !(trigger.atType() == PERIODIC))) {
           int port = configuration.getIntSetting(DISPATCH_PORT);
           trigger.suppress(port);
           return MULTI_TRIGGERS; 
    }

    return TRIGGER_OKAY;
}

As we try to collapse the last level, we run into a problem that is, unfortunately, not very apparent. The problem is that the call of clear on trigger in the first condition has a side-effect: It changes an instance variable in the class. If we duplicate that call across the other conditions, we'll do that work several times and we will definitely not be refactoring, we'll be introducing bugs.

Fortunately, there is a nice way out of this jam. We can introduce a temporary variable at the point of the first call and then use that temporary in the other places. When we do this, it is good to make the variable final or const (depending upon the language) so that we don't accidently mutate it later. The variable is supposed to represent the value of the call at that point. Listing Eight shows the resulting code.

Listing Eight

int dispatchTrigger(Trigger trigger, 
                    Configuration configuration, 
                    boolean isFallback) 
{
    final boolean triggerCleared = trigger.clear();
    if (isFallBack && !triggerCleared() && configuration.getSetting(MUTEX_DISPATCH) 
            && trigger.getType() == ONE_SHOT && trigger.atType() == PERIODIC) {
        trigger.fire();
    }
    if (isFallBack && !triggerCleared() && configuration.getSetting(MUTEX_DISPATCH) 
            && trigger.getType() == ONE_SHOT && !(trigger.atType() == PERIODIC)) {
        trigger.registerPoll();
    } 
    if (isFallBack && !triggerCleared() && !(configuration.getSetting(MUTEX_DISPATCH) 
            && trigger.getType() == ONE_SHOT && !(trigger.atType() == PERIODIC))) {
        int port = configuration.getIntSetting(DISPATCH_PORT);
        trigger.suppress(port);
        return MULTI_TRIGGERS; 
    }
    return TRIGGER_OKAY;
}

Are we completely safe doing this? It might seem that by introducing the temporary variable, we could be subtly altering behavior. After all, we are adding a value to a conditional expression that was not part of it originally. But the fact of the matter is that the value was there implicitly. It was part of the actual condition that led to the evaluation of the associated code.

Now What?

Now that we've flattened our method, we can take a final look at it. It's interesting that we have a premature exit of the function via a return statement. Did we have to account for that when we were flattening conditionals? Does our code still work the same way? Fortunately, yes. We can leave returns, exception throws, and any other early exits in place as we flatten conditionals. Each exit point is still valid.

At this point, it's good to check whether our refactoring was worthwhile. For us, it is really a toss up whether this is clearer than the original. The duplication in the conditions is rather ugly to look at. A bit more refactoring could make it clearer. We could extract methods for common expressions in the conditions and, if we give them clear evocative names, the code could become far more comprehensible.

In this case, it would be very easy to extract methods for each of the conditional sections. But many times, when we flatten conditionals, we still have quite a bit of state shared among the sections. It's not clear that we could extract methods for them. Some would be hard to name, and others would have multiple return values. But regardless of whether we extract the sections, the refactoring is useful. Sometimes you discover that there is no set of values that can cause a particular condition to be true. The associated code is dead, and it can be removed.

When you flatten conditionals in a deeply nested method, you get the benefit of having a different view of some of your most difficult to understand code. For me, it's often a refactoring that I turn to specifically to obtain that view. If I don't like how it turns out, I roll back the code to its original form. Ideally, IDEs would have this refactoring built in and easily revertible. I don't think we've gone far enough in presenting visualization tools for complicated code. Sometimes, getting the courage to move forward with a large-scale refactoring depends upon knowing just how tangled things are below the superficial complexity. If we can see beyond it, we can get a better sense of what to look out for as we start to improve the code.

Earlier in this article, I pointed out methods with deeply nested conditionals are one of the most difficult challenges in hard to understand code. It's worth trying to figure out why they exist at all.

On the one hand, we can chalk it up to simple laziness, but I think that it's actually a consequence of the nature of our work. It's easy to nest one conditional in another and convince ourselves that the code works the way we expect it to. We go away for a while and work on something else, and then we go back to the method and repeat the process. At a certain threshold, we discover that we do not understand the conditional structure of the code well enough to simplify it, but we feel that we can still make correct additions to it by adding conditions that are sub-conditions of existing ones.

Once we're past that threshold, we start to have runaway complexity and the method continues getting worse over time. But forewarned is forearmed. If we know that methods can become deeply nested over time in a slow incremental process, we can be more diligent about our refactoring each step of the way to avoid the problem.


Michael Feathers is the chief scientist at Obtiva Corp. He is a frequent lecturer on refactoring techniques and wrote the classic, Working Effectively with Legacy Code.


Related Reading


More Insights






Currently we allow the following HTML tags in comments:

Single tags

These tags can be used alone and don't need an ending tag.

<br> Defines a single line break

<hr> Defines a horizontal line

Matching tags

These require an ending tag - e.g. <i>italic text</i>

<a> Defines an anchor

<b> Defines bold text

<big> Defines big text

<blockquote> Defines a long quotation

<caption> Defines a table caption

<cite> Defines a citation

<code> Defines computer code text

<em> Defines emphasized text

<fieldset> Defines a border around elements in a form

<h1> This is heading 1

<h2> This is heading 2

<h3> This is heading 3

<h4> This is heading 4

<h5> This is heading 5

<h6> This is heading 6

<i> Defines italic text

<p> Defines a paragraph

<pre> Defines preformatted text

<q> Defines a short quotation

<samp> Defines sample computer code text

<small> Defines small text

<span> Defines a section in a document

<s> Defines strikethrough text

<strike> Defines strikethrough text

<strong> Defines strong text

<sub> Defines subscripted text

<sup> Defines superscripted text

<u> Defines underlined text

Dr. Dobb's encourages readers to engage in spirited, healthy debate, including taking us to task. However, Dr. Dobb's moderates all comments posted to our site, and reserves the right to modify or remove any content that it determines to be derogatory, offensive, inflammatory, vulgar, irrelevant/off-topic, racist or obvious marketing or spam. Dr. Dobb's further reserves the right to disable the profile of any commenter participating in said activities.

 
Disqus Tips To upload an avatar photo, first complete your Disqus profile. | View the list of supported HTML tags you can use to style comments. | Please read our commenting policy.
 

Video