Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

For the sake of readability I often find myself defining temporary variables while calling functions, such as the following code

var preventUndo = true;
doSomething(preventUndo);

The shorter version of this to this would be,

doSomething(true);

But when I come back to the code I often wonder what true refers to. Is there a convention for this kind of conundrum?

share|improve this question
5  
Do you use some kind of IDE? Most will have some sort of way of indicating what the parameters are to the function you are calling which somewhat nullifies the need to do this. –  Matthew Scharley Aug 23 '12 at 9:56
4  
I don't use an IDE, but even then I suppose the hint would require some sort of action (mouseover or moving to the line). I like to know what my code is doing just by looking at it. –  Duopixel Aug 23 '12 at 10:27
11  
If the language supports it you could use an enumeration such as doSomething( Undo.PREVENT ) –  James Poulson Aug 23 '12 at 10:29
4  
But you can define Undo = { PREVENT = true, DONT_PREVENT = false }. But in JavaScript, the convention is to do it like that: function myFunction( mandatoryArg1, mandatoryArg2, otherArgs ) { /*...*/ } and then myFunction( 1, 2, { option1: true, option2: false } ). –  xavierm02 Aug 23 '12 at 12:27
6  
It definitely depends on the language. If this were python, for example, I would suggest just using keyword arguments, e.g. doSomething(preventUndo=True) –  Joel Cornett Aug 23 '12 at 19:29

15 Answers 15

up vote 109 down vote accepted

Explaining Variables

Your case is an example of the introduce explaining variable refactoring. In short, an explaining variable is one which is not strictly necessary, but allows you to give a clear name to something, with the aim of increasing readability.

Good quality code communicates intent to the reader; and as a professional developer readability and maintainability are your #1 goals.

As such, the rule of thumb I would recommend is this: if your parameter's purpose is not immediately obvious, feel free to use a variable to give it a good name. I think this is a good practice in general (unless abused). Here's a quick, contrived example - consider:

editButton.Enabled = (_grid.SelectedRow != null && ((Person)_grid.SelectedRow).Status == PersonStatus.Active);

versus the slightly longer, but arguably clearer:

bool personIsSelected = (_grid.SelectedRow != null);
bool selectedPersonIsEditable = (personIsSelected && ((Person)_grid.SelectedRow).Status == PersonStatus.Active)
editButton.Enabled = (personIsSelected && selectedPersonIsEditable);

Boolean Parameters

Your example actually highlights why booleans in APIs are often a bad idea - on the calling side, they do nothing to explain what's happening. Consider:

ParseFolder(true, false);

You'd have to look up what those parameters mean; if they were enums, it'd be a lot more clear:

ParseFolder(ParseBehaviour.Recursive, CompatibilityOption.Strict);

Edit:

Added headings and swapped the order of the two main paragraphs, because too many people were focusing on the boolean parameters part (to be fair, it was the first paragraph originally). Also added an example to the first part.

share|improve this answer
28  
Unless you use named parameters (named arguments in .NET Framework), in which case doSomething(preventUndo: true); is clear enough. Also, creating an enum for every boolean in your API? Seriously? –  MainMa Aug 23 '12 at 10:30
11  
@MainMa: If the language you are using it not cool enough to support named arguments, using an enum is a good option. This does not mean that you should systematically introduce enums everywhere. –  Giorgio Aug 23 '12 at 11:15
18  
@MainMa - To answer that question, the point really is that you shouldn't be having booleans in your APIs (at least, your public ones). You'd need to change to a named argument syntax, which is arguably not idiomatic (at the time of writing), and in any case, a boolean parameter almost always signifies that your method does two things, and you are allowing the caller to choose. But the point of my answer was that there's nothing wrong with using explaining variables in your code; it's actually a very good practice. –  Daniel B Aug 23 '12 at 11:16
3  
It is a permanent part of the C# language, what more "mainstream acceptance" does it need? Even for someone who's not already aware of the construct (which means your C# devs haven't even read the C# docs), it's fairly obvious what it does. –  Nate C-K Aug 23 '12 at 14:27
3  
That is why MS releases those nice summaries of all the new stuff they've added in each version, I don't think it creates a lot of work to read one of these every couple of years: msdn.microsoft.com/en-us/library/bb383815%28v=vs.100%29.aspx –  Nate C-K Aug 24 '12 at 16:17

The focus of your question and of the other answers is improving readability where the function is called, which is a focus I agree with. Any specific guidelines, event "no boolean arguments", should always function as means to this end and not become and end themselves.

I think it's useful to note that this issue is mostly averted when the programming language supports named arguments / keyword arguments, like in C# 4 and Python, or where the method arguments are interleaved in the method name, like in Smalltalk or Objective-C.

Examples:

// C# 4
foo.doSomething(preventUndo: true);
# Python
foo.doSomething(preventUndo=True)
// Objective-C
[foo doSomethingWith:bar shouldPreventUndo:YES];
share|improve this answer

One should avoid passing boolean variables as arguments to functions. Because functions should do one thing at a time. By passing boolean variable the function has got two behaviors. This also creates readability problem for at a later stage or for other programmers who tend to see your code. This also creates problem in testing the function. Probably in this case you have to create two test cases. Imagine if you have a function which has got switch statement and changes its behavior based on the switch type then you have to have so many different test cases defined for it.

Whenever one comes across having a boolean argument for the function, they have to tweak the code such that they dont write boolean argument at all.

share|improve this answer
    
So you would convert a function with a boolean argument in two functions which only differ in a tiny part of their code (where the original would have an if(param) {...} else {...})? –  Paŭlo Ebermann Aug 24 '12 at 18:42

Just for a less conventional solution and since the OP mentionned Javascript a possibility is to use an associative array for mapping values. The advantage over a single boolean is that you can have as many arguments as you like and not be worried about their sequence.

var doSomethingOptions = new Array();

doSomethingOptions["PREVENTUNDO"] = true;
doSomethingOptions["TESTMODE"] = false;

doSomething( doSomethingOptions );

// ...

function doSomething( doSomethingOptions ){
    // Check for null here
    if( doSomethingOptions["PREVENTUNDO"] ) // ...
}

I realize this is a reflex of someone from a strongly-typed background and might not be so practical but consider this for originality.

Another possibility is to have an object and is also possible in Javascript. I'm not a 100% sure this is syntacly correct. Have a look at patterns such as Factory for further inspiration.

function SomethingDoer( preventUndo ) {
    this.preventUndo = preventUndo;
    this.doSomething = function() {
            if( this.preventUndo ){
                    // ...
            }
    };
}

mySomethingDoer = new SomethingDoer(true).doSomething();
share|improve this answer
1  
I think this can be better written in the dot-notation (doSomethingOptions.PREVENTUNDO) instead of the array access notation. –  Paŭlo Ebermann Aug 24 '12 at 18:46

Neither, you shouldn't write methods with boolean flags.

To quote Robert C. Martin says in his book Clean Code (ISBN-13 978-0-13-235088-4), "Passing a boolean into a function is a truly terrible practice."

To paraphrase him, the reasoning is that the fact that you have a true/false switch means that your method is most likely doing two different things (ie "Do something with undo" and "Do something without undo"), and should therefore be split into two different methods (that may internally call the same thing).

DoSomething()
{...

DoSomethingUndoable()
{....
share|improve this answer
    
I think never passing boolean arguments is a bit extreme. I buy the advice that "methods should not do two different things," but there are plenty of cases where it's reasonable, elegant, and clear to pass boolean arguments around. –  Steven Aug 23 '12 at 20:11
5  
By that logic my single HotDog constructor would need to be about 16 different methods: HotDogWithMustardRelishKrautOnion(), HotDogWithMustardNorelishKrautOnion(), and so on. Or, it could be a simple HotDogWithOptions(int options), in which I interpret the options as a bit field, but then we're back to one method that does several allegedly different things. So if I go with the first option, do I have to write a big conditional to sort out which constructor to call based on the things that would otherwise have been Boolean parameters? And if the Q uses an int, this A doesn't answer the Q. –  Caleb Aug 23 '12 at 21:32
4  
After reading all the answers and looking at my code I'm coming to the conclusion that this is the right way of doing it. doSomethingUndoable() could capture the changes and then call doSomething() –  Duopixel Aug 23 '12 at 23:38
1  
So if I have a function that does a lot of processing and has an option to send an email when it is done, I should not have a boolean argument that can prevent the email from being sent, I should create a separate function? –  yakatz Aug 23 '12 at 23:51
2  
@Caleb I think the distinction is that your example uses the booleans as an actual data field, while most uses of it are to drive the flow of execution; hence Uncle Bob's railing against it. Also, his advice is a bit on the extreme side - the next few lines in the book advise against having more than 2 parameters, which is possibly an even more extreme stance. There's method to the madness though. You are absolutely right that it doesn't answer the question though, a mistake I made in my own answer. –  Daniel B Aug 24 '12 at 12:19

Since you're asking about javascript mainly, by using coffeescript you can have a named argument like syntax, super easy:

# declare method, ={} declares a default value to prevent null reference errors
doSomething = ({preventUndo} = {}) ->
  if preventUndo
    undo = no
  else
    undo = yes

#call method
doSomething preventUndo: yes
#or 
doSomething(preventUndo: yes)

compiles to

var doSomething;

doSomething = function(_arg) {
  var preventUndo, undo;
  preventUndo = (_arg != null ? _arg : {}).preventUndo;
  if (preventUndo) {
    return undo = false;
  } else {
    return undo = true;
  }
};

doSomething({
  preventUndo: true
});

doSomething({
  preventUndo: true
});

Have some fun at http://js2coffee.org/ to try the possibilities

share|improve this answer
    
I use coffeeScript only when I need to avoid javascript's OOP insanity. This is a nice solution that I might use when coding alone, it would be hard justifying to coworker that I'm passing an object instead of a boolean for the sake of readability! –  Duopixel Aug 23 '12 at 23:49
int preventUndo = true;
doSomething(preventUndo);

A good compiler for low lavel languages such as C++ will optimize machine code of above 2 line as below:

doSomething(true); 

so it doesn't matter in performance but will increase readability.

Also it is helpful to use a variable in case it's will become variable later and may accept other values as well.

share|improve this answer

I'm fond of leveraging language features to help clarity myself. For instance in C# you can specify parameters by name:

 CallSomething(name: "So and so", age: 12, description: "A random person.");

In JavaScript, I usually prefer to use an options object as an argument for this reason:

function doSomething(args) { /*...*/ }

doSomething({ name: 'So and so', age: 12, description: 'A random person.' });

That's just my preference though. I suppose it's going to depend greatly on the method being called, and how the IDE helps the developer understand the signature.

share|improve this answer
1  
That's the "downside" to loosely typed languages. You can't really enforce a signature without some validation. I didn't know you could add names like that in JS. –  James Poulson Aug 23 '12 at 21:11
1  
@JamesPoulson: You probably did know you could do that in JS and just didn't realize it. It's all over in JQuery: $.ajax({url:'', data:''}) etc etc. –  blesh Aug 24 '12 at 13:45
    
True. It seems unusual at first sight but it's similar to the inline pseudo objects that can exist in some languages. –  James Poulson Aug 24 '12 at 13:57

What I would do in JavaScript is have the function take an object as the only parameter something like this:

function doSomething(settings) {
    var preventUndo = settings.hasOwnProperty('preventUndo') ? settings.preventUndo : false;
    // Deal with preventUndo in the normal way.
}

Then call it with:

doSomething({preventUndo: true});
share|improve this answer
    
HA!!! We both made a doSomething(x) method! lol... I didn't even notice your post until now. –  blesh Aug 23 '12 at 14:32
    
What would happend if I passed a string "noUndo", and how is it clear for the person using your signature what settings is supposed to contain without looking into the implementation? –  Nick B. Aug 23 '12 at 16:26
1  
Document your code. That's what everyone else does. It makes reading your code easier, writing it only has to happen once. –  ridecar2 Aug 23 '12 at 16:28
    
@NickB. convention is powerful. If most of your code accepts objects with arguments this is clear. –  orip Aug 27 '12 at 21:13

When you look at two classes to see how coupled they are, one of the categories is data coupling, which refers to code in one class calling methods of another class and just passing in data, like what month you want a report for, and another category is control coupling, calling methods and passing in something that controls the behaviour of the method. The example I use in class is a verbose flag or a reportType flag, but preventUndo is also a great example. As you have just demonstrated, control coupling makes it hard to read calling code and understand what's happening. It also makes the calling code vulnerable to changes in doSomething() that use the same flag to control both undo and archive, or add another parameter to doSomething() to control archiving, thus breaking your code, and so on.

The problem is that the code is too tightly coupled. Passing a bool to control behaviour is, in my opinion, a sign of a bad API. If you own the API, change it. Two methods, doSomething() and doSomethingWithUndo(), would be better. if you don't own it, write two wrapper methods yourself in code you do own, and have one of them call doSomething(true) and the other call doSomething(false).

share|improve this answer

I agree with what @GlenPeterson said:

doSomething(Undo.ALLOW); // call using a self evident enum

but also insteresting would be the following since it seems to me there are only two posibilities ( true or false )

//Two methods    

doSomething()  // this method does something but doesn't prevent undo

doSomethingPreventUndo() // this method does something and prevents undo
share|improve this answer
2  
Nice idea - I hadn't thought of that. When you get more complicated like doSomething(String, String, String, boolean, boolean, boolean) then named constants are the only reasonable solution. Well, Josh Bloch suggests a factory and constructor object as in: MyThingMaker thingMaker = MyThingMaker.new(); thingMaker.setFirstString("Hi"); thingMaker.setSecondString("There"); etc... Thing t = thingMaker.makeIt(); t.doSomething(); That's just a lot more work, so it had better be worth it. –  GlenPeterson Aug 23 '12 at 12:53
    
One problem with the latter approach is that it makes it awkward to write a method which uses doSomething but allows the caller the choice of whether to allow undo. A remedy may be to have an overload which takes the Boolean option, but also have wrapper versions which call the former version with the parameter always true or always false. –  supercat Aug 23 '12 at 20:00
    
@GlenPeterson A factory is a possibility and is possible in Javascript (mentionned by OP). –  James Poulson Aug 23 '12 at 21:10

I find this is the simplest and easiest to read:

enum Undo { ALLOW, PREVENT }

doSomething(Undo u) {
    if (Undo.ALLOW == u) {
        // save stuff to undo later.
    }
    // do your thing
}

doSomething(Undo.ALLOW);

MainMa doesn't like that. We may have to agree to disagree, or we can use a solution that Joshua Bloch proposes:

enum Undo {
    ALLOW {
        @Override
        public void createUndoBuffer() {
            // put undo code here
        }
    },
    PREVENT {
        @Override
        public void createUndoBuffer() {
            // do nothing
        }
    };

    public abstract void createUndoBuffer();
}

doSomething(Undo u) {
    u.createUndoBuffer();
    // do your thing
}

Now if you ever add an Undo.LIMITED_UNDO, your code won't compile unless you implement the createUndoBuffer() method. And in doSomething() there is no if (Undo.ALLOW == u). I've done it both ways and the second method is pretty heavyweight and a little hard to understand when the Undo enum expands to pages and pages of code, but it does make you think. I usually stick with the simpler method to replace a simple boolean with a 2-value enum until I have reason to change. When I add a third value, I use my IDE to "Find-usages" and fix everything up then.

share|improve this answer

I think your solution makes your code a bit more readable but I would avoid defining an extra variable just to make your function call clearer. Also, if you do want to use an extra variable, I would mark it as constant if your programming language supports it.

I can think of two alternatives which do not involve an extra variable:

1. Use an extra comment

doSomething(/* preventUndo */ true);

2. Use a two-valued enum instead of boolean

enum Options
{
    PreventUndo,
    ...
}

...

doSomething(PreventUndo);

You can use alternative 2 if your language supports enums.

EDIT

Of course, using named arguments is also an option, if your language supports them.

Regarding the extra coding needed by enums, it really seems negligible to me. Instead of

if (preventUndo)
{
    ...
}

you have

if (undoOption == PreventUndo)
{
    ...
}

or

switch (undoOption)
{
  case PreventUndo:
  ...
}

And even if it is a little more typing, remember that code is written once and read many times, so it can be worthwhile typing a bit more now in order to find a more readable code six months later.

share|improve this answer

Don't write code you don't need.

If you find doSomething(true) difficult to understand, you should either add a comment:

// Do something and prevent the undo.
doSomething(true);

or, if the language supports it, add the parameter name:

doSomething(preventUndo: true);

Otherwise, rely on your IDE to give you the signature of the called method:

enter image description here

Cases where it's useful

Putting an additional variable can be useful:

  1. For debugging purposes:

    var productId = this.Data.GetLastProductId();
    this.Data.AddToCart(productId);
    

    The same code can be written in a single line, but if you want to put a breakpoint before adding a product to cart in order to see if the product id is correct, writing two lines instead of one is a good idea.

  2. If you have a method with lots of parameters and each parameter goes from an evaluation. In one line, this may become totally unreadable.

    // Even with indentation, this is unreadable.
    var doSomething(
        isSomethingElse ? 0 : this.getAValue(),
        this.getAnotherOne() ?? this.default,
        (a + b + c + d + e) * f,
        this.hello ? this.world : (this.hello2 ? this.world2 : -1));
    
  3. If the evaluation of a parameter is too complicated. An example of a code I've seen:

    // Wouldn't it be easier to have several if/else's (maybe even in a separate method)?
    do(something ? (hello ? world : -1) : (programmers ? stackexchange : (com ? -1 : 0)));
    

Why not in other cases?

Why shouldn't you create additional variables in simple cases?

  1. Not because of the performance impact. This would be a very wrong assumption of a beginner developer who is micro-optimizing his app. There is no performance impact in most languages, since the compiler will inline the variable. In those languages where the compiler doesn't do that, you might gain a few microseconds by inlining it by hand, which doesn't worth it. Don't do that.

  2. But because of the risk of dissociating the name you give to the variable to the name of the parameter.

    Example:

    Let's say the original code is:

    void doSomething(bool preventUndo)
    {
        // Does something very interesting.
        undoHistory.removeLast();
    }
    
    // Later in code:
    var preventUndo = true;
    doSomething(preventUndo);
    

    Later, a developer working on doSomething notices that recently, the undo history introduced two new methods:

    undoHistory.clearAll() { ... }
    undoHistory.disable() { ... }
    

    Now, preventUndo seems not very clear. Does it mean that only the last action will be prevented? Or maybe it means that the user will not be able to use undo feature any longer? Or that the undo history will be cleared? The clearer names would be:

    doSomething(bool hideLastUndo) { ... }
    doSomething(bool removeAllUndo) { ... }
    doSomething(bool disableUndoFeature) { ... }
    

    So now you have:

    void doSomething(bool hideLastUndo)
    {
        // Does something very interesting.
        undoHistory.removeLast();
    }
    
    // Later in code, very error prone, while the signature of the method is clear:
    var preventUndo = true;
    doSomething(preventUndo);
    

What about enums?

Some other people suggested using enums. While it solves the immediate problem, it creates a bigger one. Let's take a look:

enum undoPrevention
{
    keepInHistory,
    prevent,
}

void doSomething(undoPrevention preventUndo)
{
    doTheJob();
    if (preventUndo == undoPrevention.prevent)
    {
        this.undoHistory.discardLastEntry();
    }
}

doSomething(undoPrevention.prevent);

Problems with that:

  1. It's too much code. if (preventUndo == undoPrevention.prevent)? Seriously?! I don't want to write such ifs every time.

  2. Adding an element to the enum is very tempting later, if I'm using the same enum somewhere else. What if I modify it like this:

    enum undoPrevention
    {
        keepInHistory,
        prevent,
        keepButDisable, // Keeps the entry in the history, but makes it disabled.
    }
    

    What will happen now? Will doSomething method work as expected? In order to prevent this, it would require to write this method this way from the beginning:

    void doSomething(undoPrevention preventUndo)
    {
        if (![undoPrevention.keepInHistory, undoPrevention.prevent].contains(preventUndo))
        {
            throw new ArgumentException('preventUndo');
        }
    
        doTheJob();
        if (preventUndo == undoPrevention.prevent)
        {
            this.undoHistory.discardLastEntry();
        }
    }
    

    The variant which uses booleans starts to look so nice!

    void doSomething(bool preventUndo)
    {
        doTheJob();
        if (preventUndo)
        {
            this.undoHistory.discardLastEntry();
        }
    }
    
share|improve this answer
    
Just to add, if you're using OO n°2 could be made into some sort of Builder. Also, if the language has support for enumerations you could add some semantics to the parameter being passed. –  James Poulson Aug 23 '12 at 10:25
3  
"It's too much code. if (preventUndo == undoPrevention.prevent)? Seriously?! I don't want to write such ifs every time.": what about using a good old switch statement? Also, if you use a boolean, you need an if statement anyway. Honestly, your criticism on this point seems a bit artificial to me. –  Giorgio Aug 23 '12 at 10:57
2  
@superM: I don't find it that much harder to change the name of a temporary variable used only for the call. If an enum name is changed, then it's even better, because the call code won't run anymore and throws an error right into your face. If the functionality of the function is radically changed by an enum extension, then you have to revisit all calls for further correctness, anyway, else you program on hope, not on logic. Not even to think about changing the values, e.g. negating preventUndo to allowUndo in the signature... –  Secure Aug 23 '12 at 11:24
3  
@superM keeping comments in sync with the code is very trouble-prone indeed, since you have no help from your compiler in your search for inconsistencies. –  Buhb Aug 23 '12 at 11:27
6  
"... rely on your IDE to give you the signature of the called method" - this is useful for when you're writing the code, but not so much when you're reading it. When I read code for a class, I want to know what it's doing just by looking at it. If you have to rely on the IDE for readability, your code is not self-documenting. –  Phil Aug 23 '12 at 13:31

It is not the role of the caller to define the role of arguments. I always go for the inline version, and check the signature of the called function if I have a doubt.

Variable should be named after their role in the current scope. Not The scope where they are sent.

share|improve this answer
1  
the role of the variable in the current scope is to tell what it is used for. I don't see how this contradicts OP's usage of temporary variables. –  superM Aug 23 '12 at 11:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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