Is it wrong to use a boolean parameter to determine values?

Martin Fowler quotes Kent Beck in recommending separate setOn() setOff() methods, but also says that this should not be considered inviolable:

If you pulling[sic] data from a boolean source, such as a UI control or data source, I'd rather have setSwitch(aValue) than

if (aValue) setOn(); else setOff(); 

This is an example that an API should be written to make it easier for the caller, so if we know where the caller is coming from we should design the API with that information in mind. This also argues that we may sometimes provide both styles if we get callers in both ways.

Another recommendation is to use an enumerated value or flags type to give true and false better, context-specific names. In your example, showHint and hideHint could be better.

I think you are mixing two things in your post, the API and the implementation. In both cases I don’t think there is a strong rule you can use all the time, but you should consider these two things independently (as much as possible).

Let’s start with the API, both:

public void setHint(boolean isHintOn) 

and:

public void setHintOn() public void setHintOff() 

are valid alternatives depending on what your object is supposed to offer and how your clients are going to use the API. As Doc pointed out, if your users already have a Boolean variable (from a UI control, a user action, an external, API, etc) the first option makes more sense, otherwise you are just forcing an extra if statement on the client’s code. However, if for example you are changing the hint to true when beginning a process and to false at the end the first option gives you something like this:

setHint(true) // Do your process … setHint(false) 

while the second option gives you this:

setHintOn() // Do your process … setHintOff() 

which IMO is much more readable, so I will go with the second option in this case. Obviously, nothing stops you from offering both options (or more, you could use an enum as Graham said if that makes more sense for example).

The point is that you should pick your API based on what the object is supposed to do and how the clients are going to use it, not based on how you are going to implement it.

Then you must choose how you implement your public API. Let’s say we picked the methods setHintOn and setHintOff as our public API and they share this common logic as in your example. You could easily abstract this logic through a private method (code copied from Doc):

private void setHint(boolean isHintOn){ this.layer1.visible=isHintOn; this.layer2.visible=!isHintOn; this.layer3.visible=isHintOn; } public void setHintOn(){ setHint(true); } public void setHintOff(){ setHint(false); } 

Conversely, let’s say we picked setHint(boolean isHintOn) a our API but let’s reverse your example, due to whatever reason setting the hint On is completely differente to setting it to Off. In this case we could implement it as follows:

public void setHint(boolean isHintOn){ if(isHintOn){ // Set it On } else { // Set it Off } } 

Or even:

public void setHint(boolean isHintOn){ if(isHintOn){ setHintOn() } else { setHintOff() } } private void setHintOn(){ // Set it On } private void setHintOff(){ // Set it Off } 

The point is that, in both cases, we first picked our public API and then adapted our implementation to fit the chosen API (and the constraints we have), not the other way around.

By the way, I think the same applies to the post you linked about using a boolean parameter to determine behavior, i.e. you should decide based on your particular use case rather than some hard rule (though in that particular case usually the correct thing to do is break it in multiple functions).

First things first: code is not automatically less maintainable, just because it's a bit longer. Clarity is what matters.

Now, if you're really just dealing with data, then what you have is a setter for a boolean property. In that case you might want to just store that value directly and derive the layer visibilities, i.e.

bool isBackgroundVisible() { return isHintVisible; } bool isContentVisible() { return !isHintVisible; } 

^{(I've taken the liberty to give the layers actual names - if you don't have this in your original code, I'd start with that)}

This still leaves you with the question of whether to have a setHintVisibility(bool) method. Personally, I would recommend replacing it with a showHint() and hideHint() method - both will be really simple and you won't have to change them when you add layers. It's not a clear cut right/wrong, however.

Now if calling the function should actually change the visibility of those layers, you actually have behavior. In that case, I would definitely recommend separate methods.

Boolean parameters are fine in the second example. As you already have figured out, boolean parameters are not in themselves problematic. It is switching behavior based on a flag, which is problematic.

The first example is problematic though, because the naming indicates a setter method, but the implementations seem to be something different. So you have the behavior-switching antipattern, and a misleadingly named method. But if the method actually is a regular setter (without behavior switching), then there is no problem with setState(boolean). Having two methods, setStateTrue() and setStateFalse() is just needlessly complicating things for no benefit.

Another way to solve this problem is to introduce an object to represent each hint, and have the object be responsible for determining the boolean values associated with that hint. This way you can add new permutations rather than simply having two boolean states.

For example, in Java, you could do:

public enum HintState { SHOW_HINT(true, false, true), HIDE_HINT(false, true, false); private HintState(boolean layer1Visible, boolean layer2Visible, boolean layer3Visible) { // constructor body and accessors omitted for clarity } } 

And then your caller code would look like this:

setHint(HintState.SHOW_HINT); 

And your implementation code would look like this:

public void setHint(HintState hint) { this.layer1Visible = hint.isLayer1Visible(); this.layer2Visible = hint.isLayer2Visible(); this.layer3Visible = hint.isLayer3Visible(); } 

This keeps the implementation code and the caller code concise, in exchange for defining a new data type which clearly maps strongly-typed, named intentions to the corresponding sets of states. I think that's better all around.

So my question is, if the boolean parameter is used to determine values only, but not behaviours (eg:no if-else on that parameter), is it still recommended to eliminate that boolean parameter?

When I have doubts about such things. I like to picture what the stack trace would look like.

For many years I worked on a PHP project that used the same function as a setter and getter. If you passed null it would return the value, otherwise set it. It was horrible to work with.

Here's an example a stack trace:

function visible() : line 440 function parent() : line 398 function mode() : line 384 function run() : line 5 

You had no idea of the internal state and it made debugging harder. There are a bunch of other negative side effects, but try to see there is value in a verbose function name and clarity when functions perform a single action.

Now picture working with the stack trace for a function that has A or B behavior based upon a boolean value.

function bar() : line 92 function setVisible() : line 120 function foo() : line 492 function setVisible() : line 120 function run() : line 5 

That's confusing if you ask me. The same setVisible lines yield two different trace paths.

So back to your question. Try to picture what the stack trace will look like, how it communicates to a person what is happening and ask yourself if you're helping a future person debug the code.

Here are some tips:

• a clear function name that implies intent without needing to know the argument values.
• a function performs a single action
• the name implies mutation or immutable behavior
• solve debugging challenges relative to the debugging ability of the language and tools.

Sometimes code looks excessive under a microscope, but when you pull back to the bigger picture a minimalistic approach will make it disappear. If you need it to stand out so that you can maintain it. Adding many small functions might feel overly verbose, but it improves maintainability when used broadly in a larger context.

In almost every case where you are passing a boolean parameter to a method as a flag to change the behavior of something, you should consider a more explicit and typesafe way to do this.

If you do nothing more than use an Enum that represents the state you have improved the comprehension of your code.

This example uses the Node class from JavaFX:

public enum Visiblity { SHOW, HIDE public boolean toggleVisibility(@Nonnull final Node node) { node.setVisible(!node.isVisible()); } } 

is always better than, like found on many JavaFX objects:

public void setVisiblity(final boolean flag); 

but I think .setVisible() and .setHidden() are the best solution for the situation where the flag is a boolean as it is the most explicit and least verbose.

in the case of something with multiple selections this is even more important to do it this way. EnumSet exists just for this reason.

Ed Mann has a really good blog post on just this subject. I was about to paraphrase just what he says, so not to duplicate effort I will just post a link to his blog post as an addendum to this answer.

When deciding between an approach for some interface that passes a (boolean) parameter vs. overloaded methods w/o said parameter, look to the consuming clients.

If all of usages would pass constant values (e.g. true, false) then that argues for the overloads.

If all of the usages would pass a variable value then that argues for the method with parameter approach.

If neither of those extremes apply, that means there is a mix of client usages, so you have to choose whether to support both forms, or make one category of clients to adapt to the other (what for them is a more unnatural) style.

There are two considerations to design:

• API: what interface you present to the user,
• Implementation: clarity, maintenability, etc...

They should not be conflated.

It is perfectly fine to:

• have multiple methods in the API delegate to a single implementation,
• have a single method in the API dispatch to multiple implementations depending on the condition.

As such, any argument which attempts to balance the costs/benefits of an API design by the costs/benefits of an implementation design is dubious and should be examined carefully.

On the API side

As a programmer, I will generally favor programmable APIs. Code is much clearer when I can forward a value than when I need an if/switch statement on the value to decide which function to call.

The latter may be necessary if each function expects different arguments.

In your case, thus, a single method setState(type value) seems better.

However, there is nothing worse than nameless true, false, 2, etc... those magic values have no meaning on their own. Avoid primitive obsession, and embrace strong typing.

Thus, from an API POV, I want: setState(State state).

On the implementation side

I recommend doing whatever is easier.

If the method is simple, it's best kept together. If the control flow is convoluted, it's best to separate it in multiple methods, each dealing with a sub-case or a step of the pipeline.

Finally, consider grouping.

In your example (with added whitespace for readability):

this.layer1.visible = isHintOn; this.layer2.visible = ! isHintOn; this.layer3.visible = isHintOn; 

Why is layer2 bucking the trend? Is it a feature or is it a bug?

Could it be possible to have 2 lists [layer1, layer3] and [layer2], with an explicit name indicating the reason they are grouped together, and then iterate over those lists.

For example:

for (auto layer : this.mainLayers) { // layer2 layer.visible = ! isHintOn; } for (auto layer : this.hintLayers) { // layer1 and layer3 layer.visible = isHintOn; } 

The code speaks for itself, it's clear why there are two groups and their behavior differ.

Separately from the setOn() + setOff() vs set(flag) question, I would carefully consider whether a boolean type is best here. Are you sure there will never be a third option?

It might be worth considering an enum instead of a boolean. As well as allowing extensibility, this also makes it harder to get the boolean the wrong way up, e.g.:

setHint(false) 

vs

setHint(Visibility::HIDE) 

With the enum, it will be much easier to extend when someone decides they want an 'if needed' option:

enum class Visibility { SHOW, HIDE, IF_NEEDED // New } 

vs

setHint(false) setHint(true) setHintAutomaticMode(true) // New 

According to ..., I know the importance of avoid using boolean parameters to determine a behaviour

I would suggest to re-evaluate this knowledge.

First of all, I do not see the conclusion that you are proposing in the SE question you linked. They are mostly talking about forwarding a parameter over several steps of method calls, where it is evaluated very far down the chain.

In your example, you are evaluating the parameter right in your method. In that respect, it does not differ at all from any other kind of parameter.

In general, there is absolutely nothing wrong with using boolean parameters; and obviously any parameter will determine a behaviour, or why would you have it in the first place?

Defining the question

Your title question is "Is it wrong to [...]?" - but what do you mean with "wrong"?.

According to a C# or Java compiler, it's not wrong. I am certain you're aware of that and it's not what you're asking. I'm afraid other than that, we only have n programmers n+1 different opinions. This answer presents what the book Clean Code has to say about this.

Answer

Clean Code makes a strong case against function arguments in general:

Arguments are hard. They take a lot of conceptual power. [...] our readers would have had to interpret it each time they saw it.

A "reader" here can be the API consumer. It can also be the next coder, who doesn't know what this code does yet - which could be you in a month. They will either go through 2 functions separately or through 1 function twice, once keeping true and once false in mind.
In short, use as little arguments as possible.

The specific case of a flag argument is later addressed directly:

Flag arguments are ugly. Passing a boolean into a function is a truly terrible practice. It immediately complicates the signature of the method, loudly proclaiming that this function does more than one thing. It does one thing if the flag is true and another if the flag is false!

In order to answer your questions directly:
According to Clean Code, It is recommended to eliminate that parameter.

Additional info:

Your example is rather simple, but even there you can see the simpleness propagating to your code: The no-parameter functions only do simple assignments, while the other function has to do boolean arithmetic to reach the same goal. It is trivial boolean arithmetic in this simplified example, but might be quite complex in a real situation.

I have seen a lot of arguments here that you should make it dependent on the API user, because having to do this in a lot of places would be stupid:

if (isAfterSunset) light.TurnOn(); else light.TurnOff(); 

I do agree that something non-optimal is happening here. Maybe it's too obvious to see, but your very first sentence is mentioning "the importance of avoid[sic] using boolean parameters to determine a behaviour" and that is the base for the whole question. I don't see a reason to make that thing which is bad to do easier for the API user.

I dont know if you do testing - in that case, also consider this:

Arguments are even harder from a testing point of view. Imagine the difficulty of writing all the test cases to ensure that all the various combinations of arguments work properly. If there are no arguments, this is trivial.