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.

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".
–
user281377Oct 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 FellowsOct 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.
–
user281377Oct 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.
–
Steve314Oct 11 '11 at 9:36

Splitting into 2 functions would still require a todo variable and would probably be harder to understand.
–
PubbyOct 11 '11 at 8:52

Yeah, I would do that, too, but my question was more about the usage of the "todo" flag.
–
Kelsey RiderOct 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.
–
Steve314Oct 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.
–
MattDaveyOct 11 '11 at 12:13

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.

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.

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 PechtelOct 11 '11 at 20: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:

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.
–
Kelsey RiderOct 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()
–
NeilNov 4 '11 at 10:13

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.

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.