As it currently stands, this question is not a good fit for our Q&A format. We expect answers to be supported by facts, references, or expertise, but this question will likely solicit debate, arguments, polling, or extended discussion. If you feel that this question can be improved and possibly reopened, visit the help center for guidance. Closed 11 years ago.

Which of the two, do you think, is better?

if (the_condition)
{
    variable = sth;
}
else
{
    variable = sth_else;
}

if (the_condition)
{
    variable_2.doSth();
}
else
{
    variable_2.doSthElse();
}

if (the_condition)
{
    variable = sth;
    variable_2.doSth();
}
else
{
    variable = sth_else;
    variable_2.doSthElse();
}

I ask about it because the second example is obviously shorter. On the other hand in the first example operations on different variables are separated, thus probably easier to r开发者_运维百科ead.

Do you consider any of them better? Or is it pointless to ask such a question as it does not matter?

#2

What if the condition changes? Then you'd have to update it in n places! (And it's easy to miss one, introducing subtle, hard-to-find bugs.)

I personally find #1 much harder to read, since I have to read the condition for each assignment because it might be different. With #2, I know that everything in the if body is in the context of a single condition -- you might say that the condition encapsulates a single, cohesive multi-statement chunk of behaviour.

Of course, if the statements in the body of an if are unrelated (i.e. not part of the same chunk of behaviour), then they should be in separate ifs. That's probably not the case here, though: You're setting up variables depending on an initial condition (which is likely one logical operation).

Repeating conditions for no reason is not good, neither for performance (probably doesn't matter) nor for maintenance (does matter).

Your view that operations on variables should be separated is not shared by most programmers. The common view is that control flow should be kept as simple as possible, which is the case for the second option.

Many good reasons to prefer the second option have already been given, but there's another one: the structure of the first option doesn't match the way most people think. Does your own brain work like this?

If I go to the store I'll buy milk.
If I go to the store I'll buy eggs.
If I go to the store I'll get cash from the ATM.

Or like this?

If I go to the store I'll buy milk, buy eggs, and get cash from the ATM.

The compact version; I find this most readable, and generally it would be how I approach it my code... YMMV, of course. ;)

// the safest way; it protects you from unexpected results in cases
// where the evaluation of "the_condition" has side-effects, or where
// it may be changed in the future to have them (as can often happen, 
// for instance, when a class field is changed to a property)
var theCondition = the_condition;

variable1 = theCondition ? sth           : sth_else;
variable2 = theCondition ? sth_different : sth_even_more_different;

// if evaluating "the_condition" has no side-effects, and you can be
// fairly sure it never will, then you can do this, but be careful:
variable1 = the_condition ? sth           : sth_else;
variable2 = the_condition ? sth_different : sth_even_more_different;

Not specific to any particular answer, remember (in cases where the results may be used multiple times) it's good practice to assign the results to appropriately-scoped variables (local if they are transient, or perhaps object or class (static) properties when they are more permanent) that are named in a way that reveals the conceptual nature of the results (e.g., not variable1 and variable2 :D).

By evaluating things only as often as needed, you will make your code more clear conceptually (assuming you do give good names to the variables you assign the evaluation to), and the compiled code will usually be more efficient (and will almost never be less efficient).

[Thanks to Peter Wone for suggesting in the comments that the point about multiple evaluations should be more explicit.]

It is also possible to use the following:

        switch (the_condition)
        {
            case true:
                variable1 = sth;
                variable2 = sth_else;
                break;

            case false:
                variable1 = sth_different;
                variable2 = sth_even_more_different;
                break;

        }

although I think it would be rare for that to be the appropriate solution; I mention it for completeness since I had not seen it suggested yet.

As I mentioned in my response to a comment, below, a possible example for the case statement would be if all of the following hold:

this check is within a finite state machine
the_condition represents a state of the FSM
~~the_condition's type FSM will likely be changed to an enumerated type at some point~~
Upon reflection:
- It seems to me that a Boolean conceptually representing something other than a truly binary [yes/no|off/on|true/false]-type option should be, regardless, treated as something other than a binary value since:
  - I think that, wherever feasible, code should be representing concepts rather than implementation details.
  - Choosing a Boolean value to represent a non-binary option would be an accidental feature of your implementation, rather than the conceptual nature of the option.
  - This could easily happen even in circumstances where you will likely never change the variable to an enumerated type.
  - And, this meets the feasibility condition, as representing this choice with a case is feasible here. [There is an assumption made here: either this is not in a tight loop, or the compiler optimizes the code to be no more intensive than a conditional, or that the code performs acceptably regardless.]
Example/Reasoning:
- You are writing a custom FSM that under certain conditions must choose items from one of two internal lists to feed to some well-worn implementation of an algorithm:
  - The implementation you are using only works when items are chosen, one from each list, either from front to back or from back to front.
  - A Boolean called UseFifoAlgorithm is chosen to be the property determining whether to pull items in a FIFO or LIFO manner.
  - So, in this case, UseFifoAlgorithm represents a choice of front-to-back but its negation "!UseFifoAlgorithm" does not conceptually equate to "Don't use a front-to-back algorithm to pull from the lists." but, rather, to "Use a back-to-front algorithm to pull from the lists."
  - Since negating the value would not mean the same thing as negating the concept behind it, using a negation of the value as a determinant in the code does not represent, conceptually, what is really going on, and it, therefore, seems inappropriate to use it so.
  - As the case statement is the construct which best represents (conceptually) making a choice between non-binary possibilities, and since it's been shown that this is not (conceptually) a binary choice, the case statement is a far more appropriate choice to represent the nature of the comparison.
  - Therefore, assuming that the code performs adequately to the task, the case statement is feasible, and, therefore, should be used (probably with comments above the 'true' and 'false' designating which method is chosen) rather than a conditional.

Okay, I know, a lot of text for a rare case, but this is the example that comes to my mind because I have often used Boolean values to maintain or define states or options in an attempt to avoid creating an enumerated value. In some of those cases I was just trying to "Do the Simplest Thing That Could Possibly Work", but in some cases I was making a poor design decision because I was too reluctant to created that enumerated value.

So, having shown that there are some possible cases where a Boolean really might be appropriate for such a choice, I think the biggest value of this example is that, I will be more likely to consider this as an option in such cases, and therefore will also be more likely to consider whether or not, indeed, a Boolean is appropriate: when there really is a forced choice between one of two possibilities, but the negation of one does not conceptually imply the second. Seems like in other cases it's probably better to bite the bullet and create the enum.

If for some reason it's undesirable to expose an additional enum to users of the class, or to users of a library, or what-have-you, then it's still probably worth considering using an enum internally to represent the choice, and then using an int or multiple booleans or something similar to expose the choice to the outside world.

I 'd say it depends on your business model.

If the two variables are guaranteed to always have their values set "as one", then the second approach should be used to reflect that (and possibly a comment added to reinforce this).

Otherwise, if the two conditions happen to be the same (but there is no requirement saying that they cannot diverge in the future), the first approach should be used.

No matter which of the above holds, replacing the conditional assignments with the ternary operator where convenient would also produce shorter code.

My personal preference:

T const variable = foo(v1, v2);
T2 const variable2 = bar(v1, v2);

A variable declared const cannot be changed (ah!) and therefore debugging is made so much easier because you can assume it won't change (otherwise the compiler would have complained).

In general, I would recommend writing short methods, but short does not mean "compact", it means performing few tasks.

#2 is cleaner, but I often end up doing the following if there is a clear main path in the code and the extra variable assignment doesn't generate much more work.

variable = sth_else;
variable_2 = sth_even_more_different;

if (the_condition)
{
    variable = sth;
    variable_2 = sth_different;
}

Other shorthand, which some argue is a bad practice. Don't use too much:

variable = the_condition ? sth : sth_else;
variable_2 = the_condition ? sth_different : sth_event_more_different;

I would consider the second better. Grouping the logic for a particular condition in one block allows easier maintenance in the future. At one glance of the code a reader can associate a given conditional control flow with its corresponding program logic without having to keep reading further down the page. The grouping also allows changes to happen in isolation without having to worry about updating other blocks.

The second choice is generally better. It is the control flow of the function as a whole that is typically of greater importance. Many projects have rules to keep the decision logic down to a minimum. That decision logic is a measurable quantity, McCabe's cyclomatic complexity or some variant of it such as extended cyclomatic complexity.

Comment on style
It's a good idea to get out of the habit of writing

if (the_condition)
  variable = sth;
else
  variable = sth_else;

Always use braces. Adding braces doesn't change the compiled code one iota. The cost to the author in adding those braces is minimal; with a smart editor the cost can be non-existant. The potential savings in reduction of stupid, hard to chase down bugs is immense. There are many, many projects that make code of the form above illegal because of this. Always use braces.

May I advocate another approach?

You seem to be assigning to the same set of variables in each if-body. Sets of variable sounds like at least a POD aggregate to me:

struct Foo {
    int variable;
    int variable_2;
};

Variant I

Then

const Foo alpha = {sth, sth_different},
          bravo = {sth_else, sth_even_more_different};
...
Foo foo;
if (the_condition)
    foo = alpha;
else
    foo = bravo;

// or something like  const Foo foo=get(the_condition);

Variant II

Or the more const-correct version with less mutable state and actual initialisation:

const Foo alpha = {sth, sth_different},
          bravo = {sth_else, sth_even_more_different};
...
const Foo foo = the_condition ? alpha
                              : bravo;

Variant III

Or in benchmarked, time critical, bottleneck code (but totally depends, as said, benchmarked), a good old lookup table:

const Foo foos[] = {{sth, sth_different},
                    {sth_else, sth_even_more_different}};
...
const Foo foo = foos[the_condition?0:1];

Variant IV

Depending on your actual use case, it might even be more natural to inverse the thing:

class Foo {
public:
    Foo (bool condition) : [...] {...}
};

Appendix

Enforcement of initialization

The nice thing about introducing a class/struct is that it also enables the enforcement of not forgetting an initialization:

class Foo {
    Foo (int var_a, int var_b) : var_a(var_a), var_b(var_b) {}

    const int var_a;
    const int var_b;

private:
    Foo(); // or "= delete" in upcoming standard
};

Performance

Interestingly, even an old gcc of the 3.x line will compile both variants ...

#include <iostream>
int pure() {                  
    bool cond;
    std::cin >> cond;        
    int a, b;    
    a = cond ? 0 : 2;
    b = cond ? 1 : 3;        
    std::cout << a << "," << b << std::endl;
}

struct Foo {
    int a, b;
};
int agg() {
    const Foo alpha = { 0, 1 },
              bravo = { 2, 3 };                  
    bool cond;
    std::cin >> cond;        
    const Foo result = cond ? alpha : bravo;        
    std::cout << result.a << "," << result.b << std::endl;
}

... into something that differs by only 2-6 assembly instructions, compared to 80-120 instructions in total. I lack a modern g++ right now, unfortunately. But already with this really dated compiler, using aggregates doesn't smell to me (if I find the time, I'll present the results later).

I'll add one more variant I think it as least worth considering. If you only use it in one place for a single binary condition, it's probably overkill. If you're likely to encounter greater complexity or use it a lot, it can be considerably more useful.

// type for pointer to member function
typedef void (*variable_2_type::pmf)();

// type holding value and action to execute:
struct result { 
    T value;
    pmf action;
};

// table of values/actions:
result results[] = { 
    {sth_else, &variable_2_type::doSthElse },
    {sth,      &variable_2_type::doSth }
};

// use the table:
variable = results[condition]. value;
variable_2.*(results[condition].action)();