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

Question

According to Is it wrong to use a boolean parameter to determine behavior?, I know the importance of avoid using boolean parameters to determine a behaviour, eg:

original version

public void setState(boolean flag){
    if(flag){
        a();
    }else{
        b();
    }
    c();
}

new version:

public void setStateTrue(){
    a();
    c();
}

public void setStateFalse(){
    b();
    c();
}

But how about the case that the boolean parameter is used to determine values instead of behaviours? eg:

public void setHint(boolean isHintOn){
    this.layer1.visible=isHintOn;
    this.layer2.visible=!isHintOn;
    this.layer3.visible=isHintOn;
}

I'm trying to eliminate isHintOn flag and create 2 separate functions:

public void setHintOn(){
    this.layer1.visible=true;
    this.layer2.visible=false;
    this.layer3.visible=true;
}

public void setHintOff(){
    this.layer1.visible=false;
    this.layer2.visible=true;
    this.layer3.visible=false;
}

but the modified version seems less maintainable because:

1. it has more codes than the original version

2. it cannot clearly show that the visibility of layer2 is opposite to the hint option

3. when a new layer (eg:layer4) is added, I need to add

   this.layer4.visible=false;
   

   and

   this.layer4.visible=true;  
   

   into setHintOn() and setHintOff() separately

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?

Answer 1

API design should focus on what is most useable for a client of the API, from the calling side.

For example, if this new API requires the caller to write regularly code like this

if(flag)
    foo.setStateTrue();
else
    foo.setStateFalse();

then it should be obvious that avoiding the parameter is worse than having an API which allows the caller to write

 foo.setState(flag);

The former version just produces an issue which then has to be solved at the calling side (and probably more than once). That does neither increase readability nor maintainability.

The implementation side, however, should not dictate how the public API looks like. If a function like setHint with a parameter needs less code in implementation, but an API in terms setHintOn/setHintOff looks easier to use for a client, one can implement it this way:

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);
}

So though the public API has no boolean parameter, there is no duplicate logic here, so only one place to change when a new requirement (like in the example of the question) arrives.

This works also the other way round: if the setState method from above needs to switch between two different pieces of code, that pieces of code can be refactored to two different private methods. So IMHO it does not make sense to search for a criterion for deciding between "one parameter/one method" and "zero parameters/two methods" by looking at the internals. Look, however, at the way you would like to see the API in the role of a consumer of it.

If in doubt, try using "test driven development" (TDD), that will force you to think about the public API and how to use it.

Answer 2

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.

Answer 3

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).

Answer 4

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.

Answer 5

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.

Answer 6

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.

Answer 7

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.

Answer 8

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.

Answer 9

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.

Answer 10

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.

Answer 11

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

Answer 12

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?

Answer 13

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.