3

Is it better to write

if (condition) {
    do_something(0);
} else if (other_condition) {
    do_something(1);
} else {
    do_something(2);
}

or

int variable;
if (condition) {
    variable = 0;
} else if (other_condition) {
    variable = 1;
} else {
    variable = 2;
}
do_something(variable);

?

I prefer the latter, because you only call the function write the function once, but it takes up more lines of code and will grow rapidly for functions with large numbers of arguments.

marked as duplicate by gnat, user40980, GlenH7, yannis Feb 16 '15 at 3:42

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 8
    Either way, you only call the function once. – Dan Pichelman Feb 11 '15 at 22:40
  • 2
    I think it depends on the language, the intent of changing the variables as such, and how likely you think you are going to need to change the arg's to the function. Many times, you could easily build a small hash/map object that has each of the conditions you need, keyed to that condition and then just call the variable with the value of the map. It doesn't really slim down the content, but it organizes and reduces the chance that you would miss a function call if you later need to edit the call. – scrappedcola Feb 11 '15 at 22:52
  • With 3 conditions, I agree you need to choose between the above. If only 2 conditions, I would embed the condition into the function call with a ternary operator: do_something(condition ? 0 : 1). For simple cases like that, I find that more readable. It doesn't scale well to multiple conditions, however. – Brandon Feb 13 '15 at 2:36
9

Use whichever one you think is more readable and maintainable. Depending on the language, variables and function calls involved, either one might be preferable. If I had to evaluate your two examples in isolation, I would have a slight preference for the second version because it makes it a little more obvious that do_something() only gets called once.

But in the context of the rest of your codebase, some "litmus test" questions like these may help with evaluating those criteria:

  • Is it more intuitive to say "if [condition] is true, [variable] should be 1" or "if [condition] is true, we should do_something(1)"?
  • Does the value of [variable] have a meaning of its own, or is it only meaningful as an argument to do_something()?
  • When this block of code has to change in the future, is it more likely that the branches will end up assigning different values to the variable(s), or that the branches will end up calling different functions?
  • What are the odds we'll ever want to call do_something() more than once in this block of code?
  • Will it ever make sense to move this branching logic inside of do_something(), rendering this entire question pointless?
  • How complicated could [condition] and [other_condition] potentially become?
5

Imagine a concrete example of a code which, given a price of an item, should call a method while specifying if the item is a rebate (price inferior to zero), a paid product (price superior to zero) or a free product (price equal to zero).

Your two pieces of code become:

Solution 1:

if (price < 0)
{
    this.DoSomething(PriceType.Rebate);
}
else if (price > 0)
{
    this.DoSomething(PriceType.Product);
}
else // price == 0
{
    this.DoSomething(PriceType.Free);
}

Solution 2:

priceType = PriceType.Free;

if (price < 0)
{
    priceType = PriceType.Rebate;
}
else if (price > 0)
{
    priceType = PriceType.Product;
}

this.DoSomething(priceType);

The second solution, as is, has less lines (10 versus 12), despite your assertion that "it takes up more lines of code", but the first one looks more readable. On the other hand, the second solution can be refactored to become much more readable:

Refactoring of solution 2:

private PriceType FindPriceType(price)
{
    if (price < 0)
    {
        return PriceType.Rebate;
    }
    else if (price > 0)
    {
        return PriceType.Product;
    }

    return PriceType.Free;
}

...

this.DoSomething(this.FindPriceType(price));

By separating the original method into two methods, the code becomes more readable through the introduction of an explicit name of the method.

There are also two specific cases where the second solution allows even more benefits:

Specific cases

The first case is the one where all conditions have the form: if (something == value). For instance:

if (priceType == 1)
{
    return PriceType.Rebate;
}

if (priceType == 2)
{
    return PriceType.Product;
}

if (priceType == 3)
{
    return PriceType.Free;
}

throw new NotImplementedException();

can be transformed into a much shorter:

var map = {
    1: PriceType.Rebate,
    2: PriceType.Product,
    3: PriceType.Free
}[priceType];

A second specific case is where you have not three branches, but only one if/else. Example:

Solution 1:

if (price < 0)
{
    this.DoSomething(PriceType.Rebate);
}
else
{
    this.DoSomething(PriceType.Product);
}

Solution 2:

priceType = PriceType.Product;

if (price < 0)
{
    return PriceType.Rebate;
}

this.DoSomething(priceType);

Here, the second solution can (in many languages) be transformed into a one-liner:

this.DoSomething(price < 0 ? PriceType.Rebate : PriceType.Product);
2

I recommend separating the concerns of the do_something and the conditions. There is some business logic that should be well named and encapsulated to determine what the do_something will operate on (ConditionType in my example below). Use the resulting ConditionType as a parameter to do_something.

I read a lot of code (more than I write) and I'm not a big fan of putting as much code into one line as possible. It's too easy to miss some crucial step when scanning the code.

Also, use constants, enums or your favorite stuc to avoid magic numers (0,1,2 in this case).

Here's a code sample:

   const int ConditionTypeZero = 0;
   const int ConditionTypeOne = 1;
   const int ConditionTypeDefault = 2;

...

        var conditionType = GetConditionType(condition, otherCondition);
        do_something(conditionType);

...

    private int GetConditionType(bool condition, bool otherCondition )
    {
        if (condition)
        {
            return ConditionTypeZero;
        }
        if (otherCondition)
        {
            return ConditionTypeOne;
        }
        return ConditionTypeDefault;
    }

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