Is defining a variable to name a method argument a good practice?

Question

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?

Answer 1

Explaining Variables

Your case is an example of the introduce explaining variable / extract 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.

Answer 2

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:

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

Answer 3

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

Answer 4

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

Answer 5

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.

Answer 6

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

Answer 7

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.

Answer 8

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.

Answer 9

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.

Answer 10

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

Answer 11

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

Answer 12

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];

Answer 13

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

Answer 14

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.

Answer 15

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.