Is this use of conditionals an anti-pattern?

Question

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.

Answer 1

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.

Answer 2

I think the ugliness is due to the fact that there is a lot of code in a single method, and/or variables are badly named (both of which are code smells on their own right - antipatterns are more abstract and complex things IMO).

So if you extract most of the code into lower level methods as @Bill suggests, the rest becomes clean (to me at least). E.g.

bool registrationNeeded = installSoftware(...);
if (registrationNeeded) {
  registerUser(...)
}

Or you may even get rid of the local flag completely by hiding the flag check into the second method and using a form like

calculateTaxRefund(isTaxRefundable(...), ...)

Overall, I don't see having a local flag variable as necessarily bad per se. Which option of the above is more readable (= preferable to me) depends on the number of method parameters, the names chosen, and which form is more consistent with the inner logic of the code.

Answer 3

todo is a really bad name for the variable, but I think that might be all that's wrong. It's hard to be entirely sure without the context.

Let's say that the second part of the function sorts a list, built by the first part. This should be much more readable:

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

if(requiresSorting)
{
    ...
}

However, Bill's suggestion is also correct. This is more readable still:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

Answer 4

The state machine pattern looks fine to me. The anti patterns in there are "todo" (bad name) and "lots of code".

Answer 5

It depends really. If the code guarded by todo (I hope you're not using that name for real as it's totally un-mnemonic!) is conceptually clean-up code, then you've got an anti-pattern and should using something like C++'s RAII or C#'s using construct to handle things instead.

On the other hand, if it is conceptually not a cleanup stage but rather just additional processing that is sometimes needed and where the decision to do it needs to be taken earlier, what is written is fine. Consider whether individual code chunks would be better refactored into their own functions of course, and also whether you've captured the meaning of the flag variable in its name, but this basic code pattern is OK. In particular, trying to put too much into other functions might make what is going on less clear, and that would definitely be an anti-pattern.

Answer 6

Many of the answers here would have trouble passing a complexity check, a few looked > 10.

I think this is the "anti-pattern" part of what you are looking at. Find a tool that measures the cyclomatic complexity of your code--there are plugins for eclipse. It's essentially a measurement of how hard your code is to test and involves the number and levels of code branches.

As a total guess at a possible solution, the layout of your code kind of makes me think in "Tasks", if this happens in a lot of places perhaps what you really want is a task-oriented architecture--each task being it's own object and in mid-task you have the ability to enqueue the next task by instantiating another task object and throwing it on the queue. These can be amazingly simple to set up and they reduce the complexity of certain types of code significantly--but as I said this is a total stab in the dark.

Answer 7

Using pdr's example above, as it is a nice example, I'll go one step further.

He had:

bool requiresSorting = BuildList(list);
if (requiresSorting)
    SortList(list);

So I realised that the following would work:

if(BuildList(list)) 
    SortList(list)

But isn't as clear.

So to the original question, why not have:

BuildList(list)
SortList(list)

And let SortList decide if it requires sorting?

You see your BuildList method seems to know about sorting, as it returns a bool indicating as such, but that makes no sense for a method that is designed to build a list.

Answer 8

Yes, this appears to be a problem because you have to keep tracking all the places you are marking the flag ON/OFF. It is better to include the logic just inside as a nested if condition instead of taking the logic out.

Also rich domain models, in that case just one liner will do big things inside the object.

Answer 9

If the flag is only set once then move the
...
code up to directly after the
... // some other code here
then do away with the flag.

Otherwise split the
... // lots of code here
... // some other code here
...
code out into seperate functions if possible, so it is clear this function has one responsibility which is the branch logic.

Wherever possible seperate the code within
... // lots of code here
out into two or more functions, some that do some work (which is a command) and others that either return the todo value (which is a query) or make it very explicit they are modifying it (a query using side-effects)

The code itself in not the anti-pattern going on here... I suspect that mingling branching logic with the actual doing of stuff (commands) is the anti-pattern your looking for.

Answer 10

Sometimes I find I need to implement this pattern. Sometimes you want to perform multiple checks before proceeding with an operation. For efficiency reasons, calculations involving certain checks aren't done unless it seems absolutely necessary to check. So you typically see code like:

// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(valuesExist) {
    try {
      // Attempt insertion
      trx.commit();
    } catch (DatabaseException dbe) {
      trx.rollback();
      throw dbe;
    }
  } else {
    closeConnection(db);
    throwException();
  }
} else {
  closeConnection(db);
  throwException();
}

This could be simplified by separating validation from the actual process of performing the operation, so you'd see more like:

boolean proceed = true;
// Check individual fields for proper input

if(fieldsValidated) {
  // Perform cross-checks to see if input contains values which exist in the database

  if(!valuesExist) {
    proceed = false;
  }
} else {
  proceed = false;
}

// The moment of truth
if(proceed) {
  try {
    // Attempt insertion
    trx.commit();
  } catch (DatabaseException dbe) {
    trx.rollback();
    throw dbe;
  }
} else {
  if(db.isOpen()) {
    closeConnection(db);
  }
  throwException();
}

Obviously it varies according to what you're trying to achieve, though written like this, both your "success" code and your "failure" code are written once, which simplifies logic and maintains the same level of performance. From there, would be a good idea to fit entire levels of validation inside inner methods which return boolean indications of success or failure which further simplify things, though some programmers like extremely long methods for some strange reason.

Answer 11

This is not a pattern. The most general interpretation is that you are setting a boolean variable and branching on its value later. That is normal procedural programming, nothing more.

Now, your specific example can be rewritten as:

if(cond1)
{
    ... // lots of code here
    ... // some other code here
    if (cond2)
    {
        ...
    }
}

That may be easier to read. Or maybe not. It depends on the rest of the code that you omitted. Focus on making that code more succinct.

Answer 12

Local flags used for control flow should be recognized as a form of goto in disguise. If a flag is only used within a function, it could be eliminated by writing out two copies of the function, labeling one as "flag is true" and the other as "flag is false", and having replacing every operation that sets the flag when it's clear, or clears it when it's set, with a jump between the two versions of the function.

In many cases, code that uses using a flag will be cleaner than any possible alternative that uses goto instead, but that's not always true. In some cases, using goto to skip a piece of code may be cleaner than using flags to do so [though some people might insert a certain raptor cartoon here].

I think the primary guiding principle should be that program logic flow should resemble the business logic description to the extent possible. If the business logic requirements are described in terms of states that split and merge in weird ways, having program logic do likewise may be cleaner than trying to use flags to hide such logic. On the other hand, if the most natural way of describing business rules would be to say that an action should be done if certain other actions had been done, the most natural way of expressing that may be to use a flag which gets set when performing the latter actions and is otherwise clear.