Design Patterns – Is This Use of Conditionals an Anti-Pattern?

anti-patternsdesign-patternspatterns-and-practices

I've seen this a lot in our legacy system at work – functions that go something like this:

bool todo = false;
if(cond1)
{
  ... // lots of code here
  if(cond2)
    todo = true;
  ... // some other code here
}

if(todo)
{
  ...
}

In other words, the function has two parts. The first part does some sort of processing (potentially containing loops, side effects, etc.), and along the way it might set the "todo" flag. The second part is only executed if the "todo" flag has been set.

It seems like a pretty ugly way to do things, and I think most of the cases that I've actually taken the time to understand, could be refactored to avoid using the flag. But is this an actual anti-pattern, a bad idea, or perfectly acceptable?

The first obvious refactorization would be to cut it into two methods. However, my question is more about whether there's ever a need (in a modern OO language) to create a local flag variable, potentially setting it in multiple places, and then using it later to decide whether to execute the next block of code.

Best Answer

I don't know about anti-pattern, but I'd extract three methods from this.

The first would perform some work and return a boolean value.

The second would perform whatever work is performed by "some other code"

The third would perform the auxiliary work if the boolean returned was true.

The extracted methods would probably be private if it was important that the second only (and always) be called if the first method returned true.

By naming the methods well, I hope it would make the code clearer.

Something like this:

public void originalMethod() {
    bool furtherProcessingRequired = lotsOfCode();
    someOtherCode();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

private boolean lotsOfCode() {
    if (cond1) {
        ... // lots of code here
        if(cond2) {
            return true;
        }
    }
    return false;
}

private void someOtherCode() {
    ... // some other code here
}

private void doFurtherProcessing() {
    // Do whatever is needed
}

Obviously there is debate to be had over whether early returns are acceptable, but that is an implementation detail (as is the code formatting standard).

Point is that the intent of the code becomes clearer, which is good...

One of the comments on the question suggests that this pattern represents a smell, and I would agree with that. It is worth looking at it to see if you can make the intent clearer.

Related Topic