14

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.

  • 2
    How do you refactor it? – Tamás Szelei Oct 11 '11 at 8:47
  • 13
    Assuming that todo is set in several places, according to several non-trivial non-exclusive conditions, I can hardly think of a refactoring that makes the slightest bit of sense. If there is no refactoring, there is no antipattern. Except the naming of the todo variable; should be named more expressive, like "doSecurityCheck". – user281377 Oct 11 '11 at 9:12
  • 3
    @ammoQ: +1; if things are complicated then that's how they are. A flag variable can make much more sense in some circumstances as it makes it clearer that a decision was taken, and you can search for it to find where that decision was made. – Donal Fellows Oct 11 '11 at 9:17
  • 1
    @Donal Fellows: If searching for the reason is necessary, I would make the variable a list; as long as it is empty, it's "false"; whereever the flag is set, a reason code is added to the list. So you might end with a list like ["blacklisted-domain","suspicious-characters","too-long"] that shows that several reasons applied. – user281377 Oct 11 '11 at 9:25
  • 2
    I don't think it's an anti-pattern, but it's definitely a smell – Binary Worrier Oct 11 '11 at 12:07

12 Answers 12

23

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.

  • Splitting into 2 functions would still require a todo variable and would probably be harder to understand. – Pubby Oct 11 '11 at 8:52
  • Yeah, I would do that, too, but my question was more about the usage of the "todo" flag. – Kricket Oct 11 '11 at 8:52
  • 2
    If you end up with if (check_if_needed ()) do_whatever ();, there's no obvious flag there. I think this can break code up too much and potentially harm readability if the code is reasonably simple, though. After all, the details of what you do in do_whatever may impact on how you test check_if_needed, so that it's useful to keep all the code together in the same screenful. Also, this doesn't guarantee that check_if_needed can avoid using a flag - and if it does, it'll probably use multiple return statements to do it, possibly upsetting strict single-exit advocates. – Steve314 Oct 11 '11 at 9:42
  • 3
    @Pubby8 he said "extract 2 methods from this", resulting in 3 methods. 2 methods doing the actual processing, and the original method coordinating the workflow. This would be a much cleaner design. – MattDavey Oct 11 '11 at 12:13
  • This omits the ... // some other code here in the early return case – Caleth Jun 8 '18 at 21:57
6

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.

4

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);
  • Why not just go one step further: if (BuildList(list)) SortList(list); – Phil N DeBlanc Jun 8 '18 at 15:57
2

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

  • I'm sure that's just for illustration, though. – Loren Pechtel Oct 11 '11 at 20:11
  • 1
    Agreed. What I was trying to convey is that good patterns drowned in poor code shouldn't be blamed for the quality of the code. – ptyx Oct 14 '11 at 0:07
1

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.

  • It's clearly not a cleanup--it doesn't always run. I've hit cases like this before--while processing something you may end up invalidating some sort of precalculated result. If the calculation is expensive you only want to run it if needed. – Loren Pechtel Oct 11 '11 at 20:10
1

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.

1

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.

  • And of course the next step is to ask why this is a two step process. Anywhere I see code like that I refactor to a method called BuildAndSortList(list) – Ian Oct 11 '11 at 11:26
  • This is not an answer. You changed the behavior of the code. – D Drmmr Jun 13 '18 at 15:12
  • Not really. Again, I can't believe I'm replying to something I posted 7 years ago, but what the hell :) What I was arguing is that SortList would contain the conditional. If you had a unit test that asserted that the list was only sorted if condition x was met, it would still pass. By moving the conditional into SortList, you avoid having to always write (if(something) then SortList(...)) – Ian Jun 14 '18 at 18:21
0

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.

0

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.

  • what does this post add that that existing answers are missing? – esoterik Jun 8 '18 at 20:04
  • @esoterik Sometimes the opportunity to add in a little CQRS frequently gets overlooked when it comes to flags... the logic to decide to change a flag represents a query, whereas doing work represents a command. Sometimes seperating the two can make the code clearer. Also it was worth pointing out in the code above that it can be simplified because the flag is only set in one branch. I feel flags are not an antipattern and if their name actually makes the code more expressive they are a good thing. I feel where flags are created, set and used should be close together in the code if possible. – andrew pate Jun 10 '18 at 10:38
0

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.

  • In the example you've given, I think I would prefer to have a function shouldIDoIt(fieldsValidated, valuesExist) that returns the answer. This is because the yes/no determination is all made at once, in contrast to the code that I see here at work, where the decision to proceed is scattered into a few different non-contiguous spots. – Kricket Oct 11 '11 at 13:31
  • @KelseyRider, that was precisely the point. Separating validation from execution allows you to stuff the logic into a method in order to simplify the overall logic of the program into if(isValidated()) doOperation() – Neil Nov 4 '11 at 10:13
0

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.

-1

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.

protected by gnat Jun 8 '18 at 18:06

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.