171

I have seen a practice from time to time that "feels" wrong, but I can't quite articulate what is wrong about it. Or maybe it's just my prejudice. Here goes:

A developer defines a method with a boolean as one of its parameters, and that method calls another, and so on, and eventually that boolean is used, solely to determine whether or not to take a certain action. This might be used, for example, to allow the action only if the user has certain rights, or perhaps if we are (or aren't) in test mode or batch mode or live mode, or perhaps only when the system is in a certain state.

Well there is always another way to do it, whether by querying when it is time to take the action (rather than passing the parameter), or by having multiple versions of the method, or multiple implementations of the class, etc. My question isn't so much how to improve this, but rather whether or not it really is wrong (as I suspect), and if it is, what is wrong about it.

16 Answers 16

91

Yes, this is likely a code smell, which would lead to unmaintainable code that is difficult to understand and that has a lower chance of being easily re-used.

As other posters have noted context is everything (don't go in heavy-handed if it's a one off or if the practice has been acknowledged as deliberately incurred technical debt to be re-factored later) but broadly speaking if there is a parameter passed into a function that selects specific behaviour to be executed then further step-wise refinement is required; Breaking up this function in to smaller functions will produce more highly cohesive ones.

So what is a highly cohesive function?

It's a function that does one thing and one thing only.

The problem with a parameter passed in as you describe, is that the function is doing more than two things; it may or may not check the users access rights depending on the state of the Boolean parameter, then depending on that decision tree it will carry out a piece of functionality.

It would be better to separate the concerns of Access Control from the concerns of Task, Action or Command.

As you have already noted, the intertwining of these concerns seems off.

So the notion of Cohesiveness helps us identify that the function in question is not highly cohesive and that we could refactor the code to produce a set of more cohesive functions.

So the question could be restated; Given that we all agree passing behavioural selection parameters is best avoided how do we improve matters?

I would get rid of the parameter completely. Having the ability to turn off access control even for testing is a potential security risk. For testing purposes either or the access check to test both the access allowed and access denied scenarios.

Ref: Cohesion (computer science)

  • Rob, would you give some explanation of what Cohesion is and how it applies? – Ray May 10 '12 at 11:12
  • Ray, the more I think about this the more I think that you should object to the code based on the security hole the boolean to turn of access control introduces into the application. The quality improvement to the code base will be a nice side effect ;) – Rob May 10 '12 at 13:08
  • 1
    Very nice explanation of cohesion and it's application. This really should get more votes. And I do agree with the security issue as well... though if they are all private methods it's a smaller potential vulnerability – Ray May 10 '12 at 13:39
  • 1
    Thanks Ray. Sounds like it will be easy enough to re-factor when time permits. Might be worth dropping a TODO comment in to highlight the issue, striking a balance between technical authority and sensitivity to the pressure that we are sometimes under to get things done. – Rob May 10 '12 at 14:00
  • "unmaintainable" – aaa90210 Jan 12 '18 at 2:24
141

I stopped using this pattern a long time ago, for a very simple reason; maintenance cost. Several times I found that I had some function say frobnicate(something, forwards_flag) which was called many times in my code, and needed to locate all the places in the code where the value false was passed as the value of forwards_flag. You can't easily search for those, so this becomes a maintenance headache. And if you need to make a bugfix at each of those sites, you may have an unfortunate problem if you miss one.

But this specific problem is easily fixed without fundamentally changing the approach:

enum FrobnicationDirection {
  FrobnicateForwards,
  FrobnicateBackwards;
};

void frobnicate(Object what, FrobnicationDirection direction);

With this code, one would only need to search for instances of FrobnicateBackwards. While it's possible there is some code which assigns this to a variable so you have to follow a few threads of control, I find in practice that this is rare enough that this alternative works OK.

However, there is another problem with passing the flag in this way, at least in principle. This is that some (only some) systems having this design may be exposing too much knowledge about the implementation details of the deeply-nested parts of the code (which uses the flag) to the outer layers (which need to know which value to pass in this flag). To use Larry Constantine's terminology, this design may have over-strong coupling between the setter and the user of the boolean flag. Franky though it's hard to say with any degree of certainty on this question without knowing more about the codebase.

To address the specific examples you give, I would have some degree of concern in each but mainly for reasons of risk/correctness. That is, if your system needs to pass around flags which indicate what state the system is in, you may find that you've got code which should have taken account of this but doesn't check the parameter (because it was not passed to this function). So you have a bug because someone omitted to pass the parameter.

It's also worth admitting that a system-state indicator that needs to be passed to almost every function is in fact essentially a global variable. Many of the downsides of a global variable will apply. I think in many cases it is better practice to encapsulate the knowledge of the system state (or the user's credentials, or the system's identity) within an object which is responsible for acting correctly on the basis of that data. Then you pass around a reference to that object as opposed to the raw data. The key concept here is encapsulation.

  • 8
    Really great concrete examples as well as insight into the nature of what we're dealing with and how it affects us. – Ray May 9 '12 at 23:25
  • 30
    +1. I use enums for this as much as possible. I've seen functions where extra bool parameters have been added later, and calls start to look like DoSomething( myObject, false, false, true, false ). It's impossible to figure out what the extra boolean arguments mean, whereas with meaningfully-named enum values, it's easy. – Graeme Perrow May 10 '12 at 18:42
  • 12
    Oh man, finally a good example of how to FrobnicateBackwards. Been searching for this forever. – Alex Pritchard Jun 24 '15 at 20:08
35

This is not necessarily wrong but it can represent a code smell.

The basic scenario that should be avoided regarding boolean parameters is:

public void foo(boolean flag) {
    doThis();
    if (flag)
        doThat();
}

Then when calling you'd typically call foo(false) and foo(true) depending on the exact behavior you want.

This is really a problem because it's a case of bad cohesion. You're creating a dependency between methods that is not really necessary.

What you should be doing in this case is leaving doThis and doThat as separate and public methods then doing:

doThis();
doThat();

or

doThis();

That way you leave the correct decision to the caller (exactly as if you were passing a boolean parameter) without create coupling.

Of course not all boolean parameters are used in such bad way but it's definitely a code smell and you're right to get suspicious if you see that a lot in the source code.

This is just one example of how to solve this problem based on the examples I wrote. There are other cases where a different approach will be necessary.

There is a good article from Martin Fowler explaining in further details the same idea.

PS: if method foo instead of calling two simple methods had a more complex implementation then all you have to do is apply a small refactoring extracting methods so the resulting code looks similar to the implementation of foo that I wrote.

  • 1
    Thanks for calling out the term "code smell"--I knew it smelled bad, but couldn't quite get at what the smell was. Your example is pretty much spot-on with what I'm looking at. – Ray May 10 '12 at 11:00
  • 22
    There are lots of situations where if (flag) doThat() inside foo() is legitimate. Pushing the decision about invoking doThat() to every caller forces repetition that will have to be removed if you later find out for some methods, the flag behavior also needs to call doTheOther(). I'd much rather put dependencies between methods in the same class than have to go scour all of the callers later. – Blrfl May 10 '12 at 13:46
  • @Blrfl: Yes, I think the more straightforward refactorings would be either creating a doOne and doBoth methods (for the false and true case, respectively) or using a separate enum type, as suggested by James Youngman – hugomg May 10 '12 at 19:40
  • @missingno: You'd still have the same problem of pushing redundant code out to callers to make the doOne() or doBoth() decision. Subroutines/functions/methods have arguments so their behavior can be varied. Using an enum for a truly Boolean condition sounds a lot like repeating yourself if the name of the argument already explains what it does. – Blrfl May 10 '12 at 20:19
  • 3
    If calling two methods one after the other or one method alone can be considered redundant then it's also redundant how you write a try-catch block or maybe an if else. Does it mean you will write a function to abstract all of them? No! Be careful, creating one method that all it does is calling two other methods does not necessarily represent a good abstraction. – Alex May 10 '12 at 20:46
24

First off: programming is not a science so much as it is an art. So there is very rarely a "wrong" and a "right" way to program. Most coding-standards are merely "preferences" that some programmers consider useful; but ultimately they are rather arbitrary. So I would never label a choice of parameter to be "wrong" in and of itself -- and certainly not something as generic and useful as a boolean parameter. The use of a boolean (or an int, for that matter) to encapsulate state is perfectly justifiable in many cases.

Coding decisions, by & large, should be based primarily on performance and maintainability. If performance isn't at stake (and I can't imagine how it ever could be in your examples), then your next consideration should be: how easy will this be for me (or a future redactor) to maintain? Is it intuitive and understandable? Is it isolated? Your example of chained function calls does in fact seem potentially brittle in this respect: if you decide to change your bIsUp to bIsDown, how many other places in the code will need to be changed too? Also, is your paramater list ballooning? If your function has 17 parameters, then readability is an issue, and you need to reconsider whether you are appreciating the benefits of object-oriented architecture.

  • 4
    I appreciate the caveat in the first paragraph. I was being intentionally provocative by saying "wrong", and certainly acknowledge that we're dealing in the realm of "best practices" and design principles, and that these sorts of things are often situational, and one must weigh multiple factors – Ray May 9 '12 at 23:23
  • 10
    Your answer reminds me of a quote that I can't remember the source of - "if your function has 17 parameters, you're probably missing one". – Joris Timmermans May 10 '12 at 10:25
  • I would very much agree with this one, and apply to the question to say that yes its often a bad idea to pass in a boolean flag, but it's never as simple as bad/good ... – JohnB Aug 14 '13 at 14:31
14

I think Robert C Martins Clean code article states that you should eliminate boolean arguments where possible as they show a method does more than one thing. A method should do one thing and one thing only I think is one of his mottos.

  • @dreza you are referring to Curlys Law. – MattDavey Mar 6 '13 at 10:13
  • Of course with experience you should know when to ignore such arguments. – gnasher729 Jun 25 '18 at 22:27
7

I think the most important thing here is to be practical.

When the boolean determines the entire behavior, just make a second method.

When the boolean only determines a little bit of behaviour in the middle, you might want to keep it in one to cut down on code duplication. Where possible, you might even be able to split the method in three: Two calling methods for either boolean option, and one that does the bulk of the work.

For example:

private void FooInternal(bool flag)
{
  //do work
}

public void Foo1()
{
  FooInternal(true);
}

public void Foo2()
{
  FooInternal(false);
}

Of course, in practice you'll always have a point in between these extremes. Usually I just go with what feels right, but I prefer to err on the side of less code duplication.

  • I only use boolean arguments to control behavior in private methods (as shown here). But the problem: If some dufus decides to increase the visibility of FooInternal in the future, then what? – ADTC Oct 15 '14 at 7:35
  • Actually, I'd go another route. The code inside FooInternal should be split into 4 pieces: 2 functions to handle the Boolean true/false cases, one for the work that happens before, and another for the work that happens after. Then, your Foo1 becomes: { doWork(); HandleTrueCase(); doMoreWork() }. Ideally, the doWork and doMoreWork functions are each broken down into (one or more) meaningful chunks of discrete actions (i.e. as separate functions), not just two functions for the sake of splitting. – jpaugh Jun 14 '17 at 19:44
7

I like the approach of customizing behavior through builder methods that return immutable instances. Here's how Guava Splitter uses it:

private static final Splitter MY_SPLITTER = Splitter.on(',')
       .trimResults()
       .omitEmptyStrings();

MY_SPLITTER.split("one,,,,  two,three");

The benefits of this are:

  • Superior readibility
  • Clear separation of configuration vs. action methods
  • Promotes cohesion by forcing you to think about what the object is, what it should and shouldn't do. In this case it's a Splitter. You'd never put someVaguelyStringRelatedOperation(List<Entity> myEntities) in a class called Splitter, but you'd think about putting it as a static method in a StringUtils class.
  • The instances are pre-configured therefore readily dependency-injectable. The client doesn't need to worry about whether to pass true or false to a method to get the correct behavior.
  • 1
    I'm partial to your solution as a big Guava lover and evangelist... but I can't give you a +1, because you skip over the part I'm really looking for, which is what is wrong (or smelly) about the other way. I think it's actually implicit in some of your explanations, so maybe if you could make that explicit, it would answer the question better. – Ray May 10 '12 at 11:04
  • I like the idea of separating of configuration and acton methods! – Sher10ck Aug 7 '15 at 5:01
  • Links to Guava library are broken – Josh Noe Jul 21 '17 at 16:16
3

Definitely a code smell. If it doesn't violate Single Responsibility Principle then it probably violates Tell, Don't Ask. Consider:

If it turns out not to violate one of those two principles, you should still use an enum. Boolean flags are the boolean equivalent of magic numbers. foo(false) makes as much sense as bar(42). Enums can be useful for Strategy Pattern and have the flexibility of letting you add another strategy. (Just remember to name them appropriately.)

Your particular example especially bothers me. Why is this flag passed through so many methods? This sounds like you need to split your parameter into subclasses.

2

I'm surprised no one has mentioned named-parameters.

The problem I see with boolean-flags is that they hurt readability. For example, what does true in

myObject.UpdateTimestamps(true);

do? I have no idea. But what about:

myObject.UpdateTimestamps(saveChanges: true);

Now it's clear what the parameter we're passing is meant to do: we're telling the function to save its changes. In this case, if the class is non-public, I think the boolean parameter is fine.


Of course, you can't force the users of your class to use named-parameters. For this reason, either an enum or two separate methods are usually preferable, depending on how common your default case is. This is exactly what .Net does:

//Enum used
double a = Math.Round(b, MidpointRounding.AwayFromZero);

//Two separate methods used
IEnumerable<Stuff> ascendingList = c.OrderBy(o => o.Key);
IEnumerable<Stuff> descendingList = c.OrderByDescending(o => o.Key); 
  • 1
    The question is not about what is preferable to a behavior determining flag, it's whether such a flag a smell, and if so, why – Ray May 10 '12 at 17:03
  • 2
    @Ray: I don't see a difference between those two questions. In a language where you can enforce the use of named parameters, or when you can be sure named-parameters will always be used (eg. private methods), boolean parameters are fine. If named parameters can't be enforced by the language (C#) and the class is part of a public API, or if the language doesn't support named parameters (C++), so that code like myFunction(true) might be written, it's a code-smell. – BlueRaja - Danny Pflughoeft May 10 '12 at 17:32
  • The named parameter approach is even more wrong. Without a name, one will be forced to read the API doc. With a name, you think you need not: but the paramter could be misnamend. For example, it could have been used to save (all) changes, but later the implmentation changed a bit so as to only save big changes (for some value of big). – Ingo Mar 6 '13 at 14:16
  • @Ingo I don't agree. That's a generic programming issue; you can easily define another SaveBig name. Any code can be screwed up, this kind of screw up is not particular to named parameters. – Maarten Bodewes May 11 '18 at 19:01
  • @Ingo: If you are surrounded by idiots, then you go and find a job elsewhere. That kind of thing is what you have code reviews for. – gnasher729 May 11 '18 at 19:49
2

TL;DR: don't use boolean arguments.

See below why they are bad, and how to replace them (in bold face).


Boolean arguments are very hard to read, and thus difficult to maintain. The main problem is that the purpose is generally clear when you read the method signature where the argument is named. However, naming a parameter is generally not required in most languages. So you will have anti-patterns like RSACryptoServiceProvider#encrypt(Byte[], Boolean) where the call determines what kind of encryption is to be used.

So you will get a call like:

rsaProvider.encrypt(data, true);

where the reader has to lookup the method's signature simply to determine what the hell true could actually means. Passing an integer is of course just as bad:

rsaProvider.encrypt(data, 1);

would tell you just as much (or rather: just as little). Even if you define constants to be used for the integer then users of the function may simply ignore them and keep using literal values.

The best way to solve this is to use an enumeration. If you have to pass an enum RSAPadding with two values: OAEP or PKCS1_V1_5 then you would immediately be able to read the code:

rsaProvider.encrypt(data, RSAPadding.OAEP);

Booleans can only have two values. This means that if you have a third option, then you'd have to refactor your signature. Generally this cannot be done because of backwards compatibility, so you'd have to extend any public class with another public method. This is what Microsoft finally did when they introduced RSACryptoServiceProvider#encrypt(Byte[], RSAEncryptionPadding) where they used an enumeration (or at least a class mimicking an enumeration) instead of a boolean.

It may even be easier to use a full object or interface as parameter, in case the parameter itself needs to be parameterized. In the above example you could imagine that you would have an OAEP padding parameter that itself can be configured with the hash value to use. Note that there are now 6 SHA-2 hash algorithms and 4 SHA-3 hash algorithms, so the number of enumeration values may explode if you only use a single enum as above (this is possibly the next thing Microsoft is going to find out).


Boolean parameters may also indicate that the method or class is not designed well. For instance, about every other cryptographic library doesn't use a padding flag in the method signature at all.

Determining the padding is something that is performed during creation or initialization of the class instance. That way the code can fail before a padding algorithm is chosen. For instance, many RSA keys on smart cards only support one of the two padding mechanisms. So you could create the provider and only find out that the mechanism is not supported when you'd perform the actual encryption or decryption. The underlying philosophy is called fail-fast.


Almost all software gurus that I like warn against boolean arguments. For instance, Joshua Bloch warns against them in the highly appreciated book "Effective Java". In general they should simply not be used. You could argue that they could be used if the case that there is one parameter that is easy to understand. But even then: Bit.set(Boolean) is probably better implemented using two methods: Bit.set() and Bit.unset().


If you cannot directly refactor the code you could define constants to at least make them more readable:

const boolean ENCRYPT = true;
const boolean DECRYPT = false;

...

cipher.init(key, ENCRYPT);

is much more readable than:

cipher.init(key, true);

even if you'd rather have:

cipher.initForEncryption(key);
cipher.initForDecryption(key);

instead.

1

I can't quite articulate what is wrong about it.

If it looks like a code smell, feels like a code smell and - well - smells like a code smell, it's probably a code smell.

What you want to do is:

1) Avoid methods with side-effects.

2) Handle necessary states with a central, formal state-machine (like this).

1

I agree with all the concerns of using Boolean Parameters to not determine performance in order to ; improve, Readability, Reliability, lowering Complexity, Lowering risks from poor Encapsulation & Cohesion and lower Total Cost of Ownership with Maintainability.

I started designing hardware in the mid 70's which we now call SCADA (supervisory control and data acquisition) and these were fine tuned hardware with machine code in EPROM running macro remote controls and collecting high speed data.

The Logic is called Mealey & Moore machines which we call now Finite State Machines. These also must be done in the same rules as above, unless it is a real time machine with a finite execution time and then shortcuts must be done to serve the purpose.

The data is synchronous but commands are asynchronous and the command logic follows memoryless Boolean logic but with sequential commands based on memory of previous, present and desired next state. In order for that to function in the most efficient machine language (only 64kB), great care was taken to define every process in a heuristic IBM HIPO fashion. That sometimes meant passing Boolean variables and doing indexed branches.

But now with lots of memory and ease of OOK, Encapsulation is an essential ingredient today but a penalty when code was counted in bytes for real time and SCADA machine code..

0

Taking an OO perspective, it would be nicer to use different implementations, depending on the value calculated at first.

But I don't think that's pratical all the time.

0

Its not necessarily wrong, but in your concrete example of the action depending on some attribute of the "user" I would pass through a reference to the user rather than a flag.

This clarifies and helps in a number of ways.

Anyone reading the invoking statement will realize that the result will change depending on the user.

In the function which is ultimately called you can easily implement more complex business rules because you can access any of the users attributes.

If one function/method in the "chain" does something different depending on a user attribute, it is very likely that a similar dependency on user attributes will be introduced to some of the other methods in the "chain".

0

Most of the time, I would consider this bad coding. I can think of two cases, however, where this might be a good practice. As there are a lot of answers already saying why it's bad, I offer two times when it might be good:

The first is if each call in the sequence makes sense in its own right. It would make sense if the calling code might be changed from true to false or false to true, or if the called method might be changed to use the boolean parameter directly rather than passing it on. The likelihood of ten such calls in a row is small, but it could happen, and if it did it would be good programming practice.

The second case is a bit of a stretch given that we're dealing with a boolean. But if program has multiple threads or events, passing parameters is the simplest way to track thread/event specific data. For example, a program may get input from two or more sockets. Code running for one socket may need to produce warning messages, and one running for another may not. It then (sort of) makes sense for a boolean set at a very high level to get passed through many method calls to the places where warning messages might be generated. The data can't be saved (except with great difficulty) in any sort of global because multiple threads or interleaved events would each need their own value.

To be sure, in the latter case I'd probably create a class/structure whose only content was a boolean and pass that around instead. I'd be almost certain to need other fields before too long--like where to send the warning messages, for instance.

  • An enum WARN, SILENT would make more sense, even when using a class/structure as you wrote (i.e. a context). Or indeed just configure the logging externally - no need to pass anything. – Maarten Bodewes May 11 '18 at 19:28
-2

Context is important. Such methods are pretty common in iOS. As just one often-used example, UINavigationController provides the method -pushViewController:animated:, and the animated parameter is a BOOL. The method performs essentially the same function either way, but it animates the transition from one view controller to another if you pass in YES, and doesn't if you pass NO. This seems entirely reasonable; it'd be silly to have to provide two methods in place of this one just so you could determine whether or not to use animation.

It may be easier to justify this sort of thing in Objective-C, where the method-naming syntax provides more context for each parameter than you get in languages like C and Java. Nevertheless, I'd think that a method that takes a single parameter could easily take a boolean and still make sense:

file.saveWithEncryption(false);    // is there any doubt about the meaning of 'false' here?
  • 18
    Actually I have no clue what false means in the file.saveWithEncryption example. Does it mean that it will save without encryption? If so, why in the world would the method have "with encryption" right in the name? I could understand have a method like save(boolean withEncryption), but when I see file.save(false), it is not at all obvious at a glance that the parameter indicates that it would be with or without encryption. I think, in fact, this makes James Youngman's first point, about using an enum. – Ray May 9 '12 at 23:20
  • 6
    An alternative hypothesis is that false means, don't overwirte any existing file of the same name. Contrived example I know, but to be sure you'd need to check the documentation (or code) for the function. – James Youngman May 9 '12 at 23:34
  • 5
    A method named saveWithEncryption that sometimes doesn't save with encryption is a bug. It should possiby be file.encrypt().save(), or like Javas new EncryptingOutputStream(new FileOutputStream(...)).write(file). – Christoffer Hammarström May 10 '12 at 14:09
  • 2
    Actually, code that does something else than what it says doesn't work, and is thus a bug. It does not fly to make a method named saveWithEncryption(boolean) that sometimes saves without encryption, just like it doesn't fly to make a saveWithoutEncryption(boolean) that sometimes saves with encryption. – Christoffer Hammarström May 10 '12 at 15:10
  • 2
    The method is bad, because there is obviously "doubt about the meaning of 'false' here". Anyway, i would never write a method like that in the first place. Saving and encrypting are separate actions, and a method should do one thing and do it well. See my earlier comments for better examples how to do it. – Christoffer Hammarström May 10 '12 at 17:27

protected by gnat Jan 10 '18 at 9:38

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.