39

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?

  • 26
    It is never wrong if the resulting code is more readable and maintainable ;-) I would recommend using the single method instead of the two seperate methods. – helb Jan 10 '18 at 9:54
  • 32
    You present a compelling argument that a single implementation of a method that sets these booleans will result in easier maintenance of the class and understanding of its implementation. Very well; those are legitimate considerations. But the public interface of the class needn't be deformed to accommodate them. If separate methods will make the public interface easier to understand and work with, define your setHint(boolean isHintOn) as a private method, and add public setHintOn and setHintOff methods that respectively call setHint(true) and setHint(false). – Mark Amery Jan 10 '18 at 12:21
  • 9
    I’d be very unhappy with those method names: they don’t really offer any benefit over setHint(true|false). Potato potahto. At least use something like setHint and unsetHint. – Konrad Rudolph Jan 10 '18 at 14:21
  • 5
    Possible duplicate of Is better Show() + Hide() or SetVisible(bool visible)? – 17 of 26 Jan 10 '18 at 16:42
  • 4
    @kevincline If the condition is one name, you write is at the beginning. isValid etc. So why change that for two words? Besides, "more natural" is in the eye of the beholder. If you want to pronounce it as an English sentence, then for me it would be more natural to have "if the hint is on" with a "the" tucked in. – Mr Lister Jan 10 '18 at 19:32

13 Answers 13

95

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.

  • DocBrown, would you say the appropriate dividing line is whether the setting of each state has complex and possibly non-reversible side effects? For example, if you're simply toggling a flag that does what it says on the tin, and there is no underlying state machine, you'd parameterise the different states. Whereas, for example, you would not parameterise a method like SetLoanFacility(bool enabled), because having provided a loan, it may not be easy to take it away again, and the two options may involve completely different logic - and you'd want to move to separate Create/Remove methods. – Steve Jan 10 '18 at 14:20
  • 15
    @Steve: you are still trying to design the API from the requirements you see at the implementation side. To be straight: that is totally irrelevant. Use whichever variant of the public API is easier to use from the calling side. Internally, you can always let two public methods call one private with a parameter. Or vice versa, you can let one method with a parameter switch between two private methods with different logic. – Doc Brown Jan 10 '18 at 15:11
  • @Steve: see my edit. – Doc Brown Jan 10 '18 at 15:23
  • I take all your points - I am actually thinking about it from the caller's side (hence my reference to "what it says on the tin"), and trying to formulate the appropriate rule where a caller would usually expect to use each approach. It seems to me that the rule is whether the caller expects repeated calls to be idempotent, and state transitions to be unconstrained and without complex side effects. Toggling a room light switch on and off would be parameterised, toggling a regional power station on and off would be multi-method. – Steve Jan 10 '18 at 15:47
  • 1
    @Steve So if the user needs to affirm a specific setting then Toggle() is not the correct function to provide. That's the entire point; if the caller only cares about "change it" and not "what it ends up as" then Toggle() is the option that avoids additional checking and decision-making. I wouldn't call that a COMMON case, nor would I recommend making it available without good reason, but if the user needs a toggle then I would give them a toggle. – Kamil Drakari Jan 10 '18 at 17:20
41

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.

  • 16
    In my experience, setSwitch(value) almost always results in less overall code than setOn/setOff precisely because of the if/else code quoted in your answer. I tend to curse developers who give me an API of setOn/setOff rather than setSwitch(value). – 17 of 26 Jan 10 '18 at 13:53
  • 1
    Looking at it from a similar lens: If you need to hardcode what the value will be, that's easy with either. However, if you need to set it to, say, user input, if you can just pass the value directly in, that saves a step. – Nic Hartley Jan 10 '18 at 13:59
  • @17of26 as a concrete counterexample (to show that "it depends" rather than that one is preferable to the other), in Apple's AppKit there is a -[NSView setNeedsDisplay:] method where you pass YES if a view should redraw and NO if it should not. You almost never need to tell it not to, so UIKit just has -[UIView setNeedsDisplay] with no parameter. It doesn't have the corresponding -setDoesNotNeedDisplay method. – Graham Lee Jan 10 '18 at 14:00
  • 2
    @GrahamLee, I think Fowler's argument is quite subtle and really rests on judgment. I wouldn't use a bool isPremium flag in his example, but I would use an enum (BookingType bookingType) to parameterise the same method, unless the logic for each booking was quite different. The "tangled logic" that Fowler refers to is often desirable if one wants to be able to see what the difference is between the two modes. And if they are radically different, I would expose the parameterised method externally, and implement the separate methods internally. – Steve Jan 10 '18 at 14:06
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).

  • 3
    As a side note, if it's something like the pseudo-block (one statement at the beginning, one at the end), you should probably use begin and end or synonyms, just to make it explicitly clear that's what they do, and to imply that a beginning has to have an ending and vice versa. – Nic Hartley Jan 10 '18 at 14:01
  • I agree with Nic, and would add: If you want to ensure begins and ends are always coupled, you should also provide language-specific idioms for that: RAII/scope guards in C++, using blocks in C#, with statement context managers in Python, passing the body of as a lambda or callable object (e.g. Ruby block syntax), etc. – Daniel Pryden Jan 10 '18 at 14:14
  • I agree with both points, I was just trying to give a simple example to illustrate the other case (though it's a bad example as you both pointed out :)). – jesm00 Jan 10 '18 at 14:18
2

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.

  • So tl;dr is: You recommend splitting into two functions because you won't have to change them when you add layers? If a layer4 is added, we may need to say "this.layer4.visibility = isHintOn", too, so I'm not sure I agree. If anything, that's a demerit against it, as now when a layer is added we have to edit two functions, not just one. – Erdrik Ironrose Jan 10 '18 at 10:49
  • No I (weakly) recommend it for clarity (showHint vs setHintVisibility). I mentioned the new layer just because OP was worried about it. Also, we just need to add one new method: isLayer4Visible. showHint and hideHint just set the isHintVisible attribute to true/false and that doesn't change. – doubleYou Jan 10 '18 at 10:58
  • 1
    @doubleYou, you say longer code is not automatically less maintainable. I would say that code length is one of the main variables in maintainability and clarity, overtopped only by structural complexity. Any code that becomes longer and more structurally complex ought to deserve it, otherwise simple problems receive a more complex treatment in code than they deserve, and the codebase acquires thickets of unnecessary lines (the "too many levels of indirection" problem). – Steve Jan 10 '18 at 16:20
  • @Steve I completely agree that you can over-engineer things, i.e. make code longer without making it clearer - otoh, you can always make code shorter at the cost of clarity, so there's no 1:1 relationship here. – doubleYou Jan 10 '18 at 20:14
  • @Steve Think „code golf“ — taking such code and rewriting it in more lines does usually make it clearer. „Code golf“ is an extreme, but there are still many programmers out there who think cramming everything into one clever expression is ”elegant” and maybe even faster because compilers are not optimizing good enough. – BlackJack Jan 11 '18 at 16:58
1

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.

1

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.

0

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.

  • 1
    What's confusing to me about the setVisible stack trace is that calling setVisible(true) appears to result in a call to setVisible(false) (or the other way around, depending on how you got that trace to occur). – David K Jan 11 '18 at 12:07
0

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.

0

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.

  • What if you're designing an integrated system and you are ultimately both the code producer and the "consuming client" of the code? How should a person standing in the shoes of a consuming client formulate their preference for one approach over another? – Steve Jan 10 '18 at 16:24
  • @Steve, As the consuming client, you know whether or not you're passing a constant or variable. If passing constants, prefer overloads w/o the parameter. – Erik Eidt Jan 10 '18 at 17:02
  • But I'm interested in articulating why that should be the case. Why not employ enums for a limited number of constants, since that is a lightweight syntax in most languages designed precisely for this kind of purpose? – Steve Jan 10 '18 at 17:09
  • @Steve, if we know at design time / compile time that clients would use a constant value (true/false) in all cases, then that suggests that there's really two different specific methods rather than one generalized method (that takes a parameter). I would argue against introducing generality of a parameterized method when it is not used -- it is a YAGNI argument. – Erik Eidt Jan 10 '18 at 19:33
0

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.

0

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
0

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?

0

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.

  • You've really buried the lede here: "...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." This is an interesting point, but you rather undermine your own argument in your last paragraph. – Wildcard Jan 12 '18 at 4:59
  • Buried the lede? The core of this answer is "use as little arguments as possible", which is explained in the first half of this answer. Everything after that is just additional info: refuting a contradicting argument (by another user, not OP) and something which doesn't apply to everybody. – R. Schmitz Jan 12 '18 at 10:14
  • The last paragraph is just trying to make clear that the title question is not well defined enough to be answered. OP asks if it's "wrong", but doesn't say according to whom or what. According to the Compiler? Looks like valid code, so not wrong. According to the book Clean Code? It uses a flag argument, so yes it is "wrong". However, I write "recommended" instead, because pragmatic > dogmatic. Do you think I need to make that more clear? – R. Schmitz Jan 12 '18 at 10:23
  • so wait, your defense of your answer is that the title question is too unclear to be answered? :D Okay...I actually thought the point I quoted was an interesting fresh take. – Wildcard Jan 12 '18 at 10:26
  • 1
    Vividly clear now; good job! – Wildcard Jan 15 '18 at 19:33

protected by gnat Jan 11 '18 at 9:25

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.