81

As noted by in the comments by @benjamin-gruenbaum this is called the Boolean trap:

Say I have a function like this

UpdateRow(var item, bool externalCall);

and in my controller, that value for externalCall will always be TRUE. What is the best way to call this function? I usually write

UpdateRow(item, true);

But I ask my self, should I declare a boolean, just to indicate what that 'true' value stands for? You can know that by looking the declaration of the function, but it's obviously faster and clearer if you just saw something like

bool externalCall = true;
UpdateRow(item, externalCall);

PD: Not sure if this question really fits here, if it doesn't, where could I get more info about this?

PD2: I didn't tag any language 'cause I thought it was a very generic problem. Anyway, I work with c# and the accepted answer works for c#

7
  • 11
    This pattern is also called the boolean trap May 16, 2018 at 21:15
  • 11
    If you're using a language with support for algebraic data types, I'd very much recommend a new adt instead of a boolean. data CallType = ExternalCall | InternalCall in haskell for example. May 17, 2018 at 7:30
  • 6
    I guess Enums would fill the exact same purpose; getting names for the booleans, and a bit of type safety. May 17, 2018 at 7:41
  • 2
    I disagree that declaring a boolean to indicate the meaning is "obviously clearer". With the first option, it's obvious that you will always pass true. With the second, you have to check (where is the variable defined? does its value change?). Sure, that's not a problem if the two lines are together... but somebody might decide that the perfect place to add their code is exactly between the two lines. It happens!
    – AJPerez
    May 17, 2018 at 15:54
  • Declaring a boolean is not cleaner, clearer it's just jumping over a step which is, looking at the documentation of the method in question. If you know the signature, it's less clear to add a new constant.
    – insidesin
    May 18, 2018 at 6:12

9 Answers 9

155

There isn't always a perfect solution, but you have many alternatives to choose from:

  • Use named arguments, if available in your language. This works very well and has no particular drawbacks. In some languages, any argument can be passed as a named argument, e.g. updateRow(item, externalCall: true) (C#) or update_row(item, external_call=True) (Python).

    Your suggestion to use a separate variable is one way to simulate named arguments, but does not have the associated safety benefits (there's no guarantee that you used the correct variable name for that argument).

  • Use distinct functions for your public interface, with better names. This is another way of simulating named parameters, by putting the paremeter values in the name.

    This is very readable, but leads to a lot of boilerplate for you, who is writing these functions. It also can't deal well with combinatorial explosion when there are multiple boolean arguments. A significant drawback is that clients can't set this value dynamically, but must use if/else to call the correct function.

  • Use an enum. The problem with booleans is that they are called "true" and "false". So, instead introduce a type with better names (e.g. enum CallType { INTERNAL, EXTERNAL }). As an added benefit, this increases the type safety of your program (if your language implements enums as distinct types). The drawback of enums is that they add a type to your publicly visible API. For purely internal functions, this doesn't matter and enums have no significant drawbacks.

    In languages without enums, short strings are sometimes used instead. This works, and may even be better than raw booleans, but is very susceptible to typos. The function should then immediately assert that the argument matches a set of possible values.

None of these solutions has a prohibitive performance impact. Named parameters and enums can be resolved completely at compile time (for a compiled language). Using strings may involve a string comparison, but the cost of that is negligible for small string literals and most kinds of applications.

17
  • 7
    I just learned that "named arguments" exist. I think this is by far the best suggestion. If you can ellaborate a bit on that option (so that other persons don't need too google about it) I'll accept this answer. Also any hints on performance if you can...Thanks May 16, 2018 at 14:16
  • 6
    One thing that can help alleviate the problems with short strings is to use constants with the string values. @MarioGarcia There's not much to elaborate on. Aside from what's mentioned here, most of the details will depend on the particular language. Was there something particular you wanted to see mentioned here?
    – jpmc26
    May 16, 2018 at 16:45
  • 8
    In languages where we're talking about a method and the enum can be scoped to a class I generally use an enum. It is completely self documenting (in the single line of code that declares the enum type, so it is lightweight too), completely prevents the method from being called with a naked boolean, and the impact on the public API is negligible (since the identifiers are scoped to the class).
    – davidbak
    May 16, 2018 at 17:38
  • 4
    Another shortcoming of using strings in this role is that you can't check their validity at compile time, unlike enums.
    – Ruslan
    May 16, 2018 at 18:40
  • 33
    enum is the winner. Just because a parameter can only have one of two possible states, that doesn't mean that it should automatically be a boolean.
    – Pete
    May 17, 2018 at 9:12
39

The correct solution is to do what you suggest, but package it into a mini-facade:

void updateRowExternally() {
  bool externalCall = true;
  UpdateRow(item, externalCall);
}

Readability trumps micro-optimization. You can afford the extra function call, certainly better than you can afford the developer effort of having to look up the semantics of the Boolean flag even once.

10
  • 9
    Is this encapsulation really worth it when I only call that funcion once in my controller? May 16, 2018 at 13:17
  • 14
    Yes. Yes, it is. The reason, as I wrote above, is that the cost (a function call) is smaller than the benefit (you save a nontrivial amount of developer time because nobody ever has to look up what the truedoes.) It would be worthwhile even if it cost as much CPU time as it saves developer time, since CPU time is way cheaper. May 16, 2018 at 13:21
  • 8
    Also any competent optimizer should be able to inline that for you so you shouldn't lose anything in the end.
    – firedraco
    May 16, 2018 at 16:54
  • 33
    @KilianFoth Except that's a false assumption. A maintainer does need to know what the second parameter does, and why it's true, and almost always this relates to the detail of what you're trying to do. Hiding functionality in death-by-a-thousand-cuts tiny functions will decrease maintainability, not increase it. I've seen it happen myself, sad to say. Excessive partitioning into functions can actually be worse than a huge God Function.
    – Graham
    May 16, 2018 at 19:41
  • 6
    I think UpdateRow(item, true /*external call*/); would be cleaner, in languages that allow that comment syntax. Bloating your code with an extra function just to avoid writing a comment doesn't seem worth it for a simple case. Maybe if there were lots of other args to this function, and / or some cluttered + tricky surrounding code, it would start to appeal more. But I think if you're debugging you're going to end up checking the wrapper function to see what it does, and have to remember which functions are thin wrappers around a library API, and which actually have some logic. May 17, 2018 at 6:15
25

Say I have a function like UpdateRow(var item, bool externalCall);

Why do you have a function like this?

Under what circumstances would you call it with the externalCall argument set to different values?

If one is from, say, an external, client application and the other is within the same program (i.e. different code modules), then I'd argue that you should have two different methods, one for each case, possibly even defined on distinct Interfaces.

If, however, you were making the call based on some datum taken from an non-program source (say, a configuration file or database read), then the Boolean-passing method would make more sense.

1
  • 6
    I agree. And just to hammer the point being made for other readers, an analysis can be made of all callers, who will pass either true, false, or a variable. If none of the latter is found the I would argue for two separate methods (w/o boolean) over one with the boolean -- it is a YAGNI argument that the boolean introduces unused/unecessary complexity. Even if there are some callers are passing variables, I might still provide the (boolean) parameter-free versions to use for callers passing a constant -- it is simpler, which is good.
    – Erik Eidt
    May 16, 2018 at 14:40
17

While I agree it's ideal to use a language feature to enforce both readability and value-safety, you can also opt for a practical approach: call-time comments. Like:

UpdateRow(item, true /* row is an external call */);

or:

UpdateRow(item, true); // true means call is external

or (rightly, as suggested by Frax):

UpdateRow(item, /* externalCall */true);
6
  • 1
    No reason to downvote. This is a perfectly valid suggestion.
    – Suncat2000
    May 16, 2018 at 20:21
  • Appreciate the <3 @Suncat2000!
    – bishop
    May 16, 2018 at 20:28
  • 11
    A (likely preferable) variant is just putting the plain argument name there, like UpdateRow(item, /* externalCall */ true ). Full-sentence comment is much harder to parse, it's actually mostly noise (especially the second variant, which is also very loosely coupled with the argument).
    – Frax
    May 16, 2018 at 20:51
  • 1
    This is by far the most appropriate answer. This is precisely what comments are intended for.
    – Sentinel
    May 16, 2018 at 21:43
  • 9
    Not a big fan of comments like this. Comments are prone to becoming stale if left untouched when refactoring or making other changes. Not to mention as soon as you have more than one bool param this gets confusing fast.
    – John V.
    May 17, 2018 at 4:47
11
  1. You can 'name' your bools. Below is an example for an OO language (where it can be expressed in the class providing UpdateRow()), however the concept itself can be applied in any language:

    class Table
    {
    public:
        static const bool WithExternalCall = true;
        static const bool WithoutExternalCall = false;
    

    and in the call site:

    UpdateRow(item, Table::WithExternalCall);
    
  2. I believe Item #1 is better but it doesn't force the user use new variables when using the function. If type safety is important for you, you can create an enum type and make UpdateRow() accept this instead of a bool:

    UpdateRow(var item, ExternalCallAvailability ec);

  3. You can modify the name of the function somehow that it reflects the meaning of the bool parameter better. Not very sure but maybe:

    UpdateRowWithExternalCall(var item, bool externalCall)

4
  • 8
    Don't like #3 as now if you call the function with externalCall=false, its name makes absolutely no sense. The rest of your answer is good. May 16, 2018 at 16:49
  • Agreed. I wasn't very sure but it may be a viable option for some other function. I
    – simurg
    May 17, 2018 at 18:21
  • @LightnessRacesinOrbit Indeed. It's safer to go for UpdateRowWithUsuallyExternalButPossiblyInternalCall. May 17, 2018 at 19:26
  • @EricDuminil: Spot on 😂 May 17, 2018 at 22:59
11

Another option I haven’t read here yet is: Use a modern IDE.

For example IntelliJ IDEA prints the variable name of the variables in the method you’re calling if you’re passing a literal such as true or null or username + “@company.com. This is done in a small font so it doesn’t take up too much space on your screen and looks very different from actual code.

I'm still not saying it's a good idea to throw in booleans everywhere. The argument saying you read code far more often than you write is often very strong, but for this particular case it does heavily depend on the technology you (and your colleagues!) use to support your job. With an IDE it's far less of a problem than with for example vim.

Modified example of part of my test setup: enter image description here

4
  • 5
    This would only be true if everyone on your team only ever used an IDE to read your code. Notwithstanding the prevalence of non-IDE editors, you also have code review tools, source control tools, Stack Overflow questions, etc., and it's vitally important that code remain readable even in those contexts. May 17, 2018 at 17:56
  • @DanielPryden sure, hence my ‘and your colleagues’ comment. It’s common to have a standard IDE in companies. As for other tools, I would argue it’s entirely possible those kinds of tools also support this or will in the future, or that those arguments simply don’t apply (like for a 2 person pair programming team) May 18, 2018 at 0:35
  • 2
    @Sebastiaan: I don't think I agree. Even as a solo developer, I regularly read my own code in Git diffs. Git is never going to be able to do the same kind of context-aware visualization that an IDE can do, nor should it. I'm all for the use of a good IDE to help you write code, but you should never use an IDE as an excuse to write code that you wouldn't have written without it. May 18, 2018 at 11:50
  • 1
    @DanielPryden I’m not advocating writing extremely sloppy code. It’s not a black and white situation. There may be cases where in the past for a specific situation you would be 40% in favor of writing a function with a boolean and 60% not and you ended up not doing it. Maybe with good IDE support now the balance is 55% writing it like that and 45% not. And yes you may still need to read it outside of the IDE sometimes so it’s not perfect. But it’s still a valid trade-off against an alternative, like adding another method or enumeration to clarify the code otherwise. May 18, 2018 at 11:59
6

2 days and no ones mentioned polymorphism?

target.UpdateRow(item);

When I'm a client wanting to update a row with an item I don't want to think about the name of the database, the URL of the micro-service, the protocol used to communicate, or whether or not an external call is going to need to be used to make it happen. Stop pushing these details on me. Just take my item and go update a row somewhere already.

Do that and it becomes part of the construction problem. That can be solved many ways. Here's one:

Target xyzTargetFactory(TargetBuilder targetBuilder) {
    return targetBuilder
        .connectionString("some connection string")
        .table("table_name")
        .external()
        .build()
    ; 
}

If you're looking at that and thinking "But my language has named arguments, I don't need this". Fine, great, go use them. Just keep this nonsense out of the calling client that shouldn't even know what it's talking to.

It should be noted that this is more than a semantic problem. It's also a design problem. Pass in a boolean and you are forced to use branching to deal with it. Not very object oriented. Have two or more booleans to pass in? Wishing your language had multiple dispatch? Look into what collections can do when you nest them. The right data structure can make your life so much easier.

2

If you can modify the updateRow function, perhaps you can refactor it into two functions. Given that the function takes a boolean parameter, I suspect it looks like this:

function updateRow(var item, bool externalCall) {
  Database.update(item);

  if (externalCall) {
    Service.call();
  }
}

That can be a bit of a code smell. The function might have dramatically different behavior depending on what the externalCall variable is set to, in which case it has two different responsibilities. Refactoring it into two functions that only have one responsibility can improve readability:

function updateRowAndService(var item) {
  updateRow(item);
  Service.call();
}

function updateRow(var item) {
  Database.update(item);
}

Now, anywhere you call these functions, you can tell at a glance whether the external service is being called.

This isn't always the case, of course. It is situational and a matter of taste. Refactoring a function that takes a boolean parameter into two functions is usually worth considering, but isn't always the best choice.

0

If UpdateRow is in a controlled codebase, I would consider strategy pattern:

public delegate void Request(string query);    
public void UpdateRow(Item item, Request request);

Where Request represents some kind of DAO (a callback in trivial case).

True case:

UpdateRow(item, query =>  queryDatabase(query) ); // perform remote call

False case:

UpdateRow(item, query => readLocalCache(query) ); // skip remote call

Usually, endpoint implementation would be configured on a higher level of abstraction and I would just use it here. As a useful free side-effect, this gives me an option to implement another way to access remote data, without any change to the code:

UpdateRow(item, query => {
  var data = readLocalCache(query);
  if (data == null) {
    data = queryDatabase(query);
  }
  return data;
} );

In general, such inversion of control ensures lower coupling between your data storage and model.

Not the answer you're looking for? Browse other questions tagged or ask your own question.