Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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?

Edit: 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.

share|improve this question
2  
How do you refactor it? –  Tamás Szelei Oct 11 '11 at 8:47
7  
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
1  
@ammoQ - so long as performance isn't an issue, that may be a good idea - but in a lot of contexts it's excessive, and it doesn't add much value in terms of readability or debugging compared with either comments (with any change of the flag triggering a breakpoint) or write-reason-to-log-file calls. In fact, writing reasons to a log file as they're detected is likely to be much more useful if problems occur on a customer site. –  Steve314 Oct 11 '11 at 9:36

9 Answers 9

up vote 17 down vote accepted

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

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

The second 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();
    if (furtherProcessingRequired) {
        doFurtherProcessing();
    }
    return;
}

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

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.

share|improve this answer
    
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. –  Kelsey Rider Oct 11 '11 at 8:52
1  
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
2  
@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

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

share|improve this answer
    
I'm sure that's just for illustration, though. –  Loren Pechtel Oct 11 '11 at 20:11
    
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

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
    
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. –  Kelsey Rider 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

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.

share|improve this answer
    
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

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.

share|improve this answer

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.

share|improve this answer
    
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

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);
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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