Which is the better method to use "if else" statements

I find the latter more readable. Which is the most efficient depends on what your compiler produces. Check the assemble listing for an answer.

Meindert

It is up to /you/ to decide which is the most readable.

As to which is the most efficient, it depends on the target processor and the compiler. If you think that the efficiency of this code is important, then /you/ have to compile it and view the generated assembly. It is quite possible that the compiler will generate the same target code for both functions. But it may be a little different, and that may affect the speed of the function - depending on the target processor, the differences between "branch taken" and "branch skipped" times, branch prediction logic (if any), caching, and the likelihood of the different conditions being true or false.

Almost certainly, the efficiency is irrelevant - if not, you have much bigger problems in store for you. So pick the format you think is most readable and easiest to maintain.

First, how I would rewrite each of your examples (indentation and spacing):

uint16_t Func1( uint16_t int1, uint16_t int2, uint16_t int3 ) { if (int1 == 32) { if (int2 == 16) { if (int3 == 8) { return 0; } else { return 1; } } else { return 2; } } else { return 3; } }

uint16_t Func1( uint16_t int1, uint16_t int2, uint16_t int3 ) { if (int1 != 32) { return 3; }

if (int2 != 16) { return 2; }

if (int3 != 8) { return 1; }

return 0; }

[I trade vertical space differently than many would :-/ ]

Next, some alternative ways of writing it:

uint16_t Func1( uint16_t int1, uint16_t int2, uint16_t int3 ) { if (int1 == 32) && (int2 == 16) && (int3 == 8) { return 0; }

if (int1 == 32) && (int2 == 16) && (int3 != 8) { return 1; }

if (int1 == 32) && (int2 != 16) { return 2; }

if (int1 != 32) { return 3; }

/* CAN'T HAPPEN */ }

uint16_t Func1( uint16_t int1, uint16_t int2, uint16_t int3 ) { if (int1 == 32) && (int2 == 16) { return (int3 == 8) ? 0 : 1; }

if (int1 == 32) { return 2; } else { return 3; }

/* CAN'T HAPPEN */ }

[I hope I haven't mangled the logic while cutting and pasting :< ]

The point being that you need to think about what you are trying to accomplish. Presumably, you aren't testing generic variables for specific generic values (unless this is a homework assignment). Rather, I would assume something more along the lines of:

result_t /* SAFE_TO_CHANGE or otherwise */ safe_to_change_cutting_tool( motorstate_t motor, /* STOPPED or otherwise */ toolstate_t toolhead, /* RETRACTED or otherwise */ carriage_t position /* HOME or otherwise */ ) { /* if motor is still running, don't do anything! */ if (motor != STOPPED) { return MECHANISM_IN_MOTION; }

/* motor == STOPPED */ if (toolhead != RETRACTED) { return TOOL_STUCK_IN_WORKPIECE; }

/* motor == STOPPED, toolhead == RETRACTED */ if (position != HOME) { return INCORRECT_CARRIAGE_POSITION; }

return SAFE_TO_CHANGE; }

Putting some symbolic labels on the conditions makes a difference in how a human would read it (with some thought, you can come up with a different set of labels that would want to be read using a different "code layout" -- think about it!)

While reading the code, I like to know exactly what the conditions are that apply to the stanza that I am reading at the time. That can be done explicitly (e.g., list all of the conditions in the "if") or implicitly (e.g., the above example assigns a logical priority to the conditions which are being tested: the motor being in motion overrides all other considerations, etc.)

As to the efficiency of the generated code, that depends on your compiler. Some are actually very clever and will come up with essentially the same code for each version! (YMMV).

Unless you have some overwhelming reason why "efficiency" (size? speed??) is of paramount importance, try to write the code so it is least likely to confuse a reader seeing it for the first time. And, feel free to add commentary explaining why you are making certain tests. (as well as "notes to yourself" about what previous tests have told you about the state of things "at this point" in the function body -- i.e., my "/* motor == STOPPED */" comments...)

A piece of code shall have only one entry point and only one exit point.

Vladimir Vassilevsky DSP and Mixed Signal Design Consultant

As with most "coding rules", I *really* disagree with that! :>

(I've made a goal of routinely loosening all of the "rules" that I've previously thought to have been "written in stone")

Write the code so it is easy to understand. Don't force extra *syntax* on the code just to make it fit some (well intended, though arbitrary) rule.

E.g., how do you handle:

void task(void) { while (FOREVER) { lamp(ON); sleep(ON_TIME); lamp(OFF); sleep(OFF_TIME); } }

and still satisfy the "shall" in your rule?

There is one other way to write this that I tend to use when coding. One of the "guides" to coding I read many years ago, perhaps in college said the smaller of the two choices in an if then else should be first. This makes it easier for the eye to scan the code and glean structure from the shape... a bit like the way I learned to read... look for the outline of the words rather than parsing every letter.

Also,, it is not uncommon for the else and the if to be put on the same line to show that the "if" is the only statement within the else. The result is very clean, simple and uses much less space streamlining the code to the point that the structure is very clear to the eye. Oh, and you don't need all those parens for single statements.

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if (int1 !=3D 32) return 3; else if (int2 !=3D 16) return 2; else if (int3 !=3D 8) return 1; else return 0; }

Rick

=A0OR =A0=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D

Only small minds follow rules blindly. The reason for this rule is to facilitate debugging and allow breakpoints to be set at the single exit of a routine. All of the tools I have used all me to set a breakpoint at the final semicolon which is where the actual exit from the routine is found.

Rick

A piece of code shall have only one entry point unless it is a coroutine, which is fairly specialised programming because C doesn't support coroutines well.

A piece of code shall have only one exit point unless it is clearer and neater to have more than one exit.

In my mind, the neatest way to write this code is:

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if (int1 != 32) return 3; if (int2 != 16) return 2; if (int3 != 8) return 1; return 0; }

Of course, parameter names and the various numbers should be replaced by sensible names and symbolic constants, and you may need comments (if the names are not clear enough).

That breaks another rule of good C programming - /always/ use brackets with "if" statements. But by being neat and compact, it is easy to see what is going on - and that trumps most other rules.

static void callback_on(void) { lamp(ON); timer(callback_off, ON_TIME); }

static void callback_off(void) { lamp(OFF); timer(callback_on, OFF_TIME); }

void main(void) { callback_on(); }

--
Gemaakt met Opera's revolutionaire e-mailprogramma:  
http://www.opera.com/mail/
(remove the obvious prefix to reply by mail)

Why waste all those linefeeds when you can simply write

uint16_t func(uint16_t i1, uint16_t i2, uint16_t i3) {return i1 != 32 ? 3: i2 != 16 ? 2: i3 !=8 ? 1 : 0;}

? ;-)

A nitpick. The else is completely unnecessary. Regarding the braces, I use them if the true statement is not on the same line as the if, even if it is only on statement.

--------------------------------------- Posted through

I agree on that regarding the braces. I think the style with slightly indented if "true" statements on their own line is horrible - they are hard to spot, and very easy to get wrong. And when someone modifies it to have two statements, it's easy to break. If the "true" statement is short and clearly a single statement (such as "return 3;") with no possibility of being a macro, then it can be neater to have it on the same line. If you need a new line, use braces and indent. And if you need an "else", then you /always/ want to use braces and indent - code using multiple if's and else's without braces and indents is right out.

Of course, there are other ways to format you if's. But there is only one /good/ way :-)

This is the sort of discussion in which reasonable people can reasonably disagree.

I know that is the conventional structured programming wisdom, but I have seen far too many situations in which obeying that dictum results in code getting a lot harder to understand. The point of that rule (which I follow as often as I can) is to make sure you don't hack an unstructured GOTO via a return statement stuck in the middle of a module. My own guideline is that there should be only a single exit point OR exit points should be in a highly regular pattern (e.g., a return inside every if statement). In other words, regular structure is what matters to help in understanding the code.

If you want to obey the single-exit rule, you can usually use a chain of "else if" clauses. Depends upon your taste and training. The downside is you often need to keep a variable around to store the return value.

The other responses in this thread have put forth many good ideas. Which ideas are best depends on the context. But I will add one tradeoff rule to the mix: Minimize the number of levels of if statement nesting to the extent practical. If you have more than 3 or 4 levels of nesting, you're asking for bugs. A chain of "else if" clauses probably doesn't count as nesting so long as there is no plain "else" the chain (in effect, an if-else chain without an intervening else is a more generalized form of a switch statement).

Phil Koopman -- snipped-for-privacy@cmu.edu --

Author of: Better Embedded System Software, Drumnadrochit Press 2010

This 3rd choice for form gets my vote too. Embedding the else-if logic in the RETURN is less clear, and more prone to later editing errors, and nesting if/elses does not make the real priority tree clear - and will likely be slower/larger, if that matters.

One-exit 'rules' make less sense the lower level your code is. Indeed, I've just speed-optimized a routine, by making the most frequent test, the earliest exit. This only mattered in one small function, called the most.

-jg

You can also micro-optimize code by ordering expressions in conditionals intelligently (instead of in the order that "reads easiest") -- assuming they are commutative.

E.g., if (sex == MALE) && (SocialSecurityNumber == 0) vs. if (SocialSecurityNumber == 0) && (sex == MALE)

(absurd example)

These are the kinds of optimizations that the compiler can't make for you as they require knowledge of the application and the values likely to be encountered by the code therein. But, the gains can be substantial!

This is a BINARY LOGICAL function. No need for if-then-elses.

:-)

//---------------------

static inline u16 eq(const u16 x, const u16 param) { u16 foo; return x&((((foo|=(foo|=(foo|=(foo|=(foo=x&(~(18)>>4)>>2)>>1)&1)^1)1)&eq(x1,4); return ((u^16)|((v>>2)^8)|(u>>1)&(eq(x2,3)^8))>>3; } //--------------------------------

Vladimir Vassilevsky DSP and Mixed Signal Design Consultant

I wouldn't waste so much vertical space on block start and end braces. More like this, instead:

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if(int1 == 32) { if(int2 == 16) { if(int3 == 8) { return 0; } else { return 1; } } else { return 2; } } else { return 3; } }

That's if I were tempted to keep the braces, which I may be ... or not. If I knew, beforehand, that the only purpose would be to return specific values and that's all... then I might simply eliminate some of them (keeping others to ensure readability as to which parts matched with which.)

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if ( int1 == 32 ) { if ( int2 == 16 ) { if ( int3 == 8 ) return 0; else return 1; } else return 2; } else return 3; }

I tend to dislike if..else.. statements as shown above. For example, although a close reading of the code does tell me that if int1==32 and int2==16 does always return an explicit value, a cursory and not-so-close look at the inner conditional instead looks to me as though it could "fall through." In other words, when skimming the code quickly I might have to stop myself for a moment and read some more of the code to understand that the inner if cannot fall out and wind up at the bottom of the function where no return statement and explicit value is provided, at all.

This also applies to the outer if. I like to see that a function returning a value _always_ has an unconditional return present with an explicit result being provided. Makes me feel safer on first blush, even if a closer reading would also inform me.

That said, I would probably "improve" it this way:

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if ( int1 == 32 ) { if ( int2 == 16 ) return int3 == 8? 0:1; return 2; } return 3; }

It is immediately clear to me that the function must return some explicit value, now. The 'return 3' is unconditional, should everything else fail. And I can easily see that if int1==32, then it must also return some _other_ explicit value... at least 2, if not something else.

Following the above comments I already made, I might recode that as:

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if ( int1 != 32 ) return 3; else if ( int2 != 16 ) return 2; return int3 == 8? 0:1; }

or as,

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if ( int1 != 32 ) return 3; if ( int2 != 16 ) return 2; return int3 == 8? 0:1; }

By the time I'm worrying about what exactly the code actually does, I've already noted the return statement doesn't allow continuation and so the choice between consecutive if statements and an if..else is moot, to me. They are the same and similarly readable so I wouldn't care much. Which means I would just be consistent in the coding, then, and do the same elsewhere whichever choice I decided upon.

So between the following two cases:

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if ( int1 == 32 ) { if ( int2 == 16 ) return int3 == 8? 0:1; return 2; } return 3; }

uint16_t Func1(uint16_t int1, uint16_t int2, uint16_t int3) { if ( int1 != 32 ) return 3; if ( int2 != 16 ) return 2; return int3 == 8? 0:1; }

I would probably choose the latter one. It is clearer to me.

Jon

Then you have to define in stone a rule which explains what is "easy to understand", as 99.9% of people have no taste neither common sense.

Don't

lamp.toggle(ON_TIME, OFF_TIME, FOREVER);

//--------------

Vladimir Vassilevsky DSP and Mixed Signal Design Consultant

But that's my whole point: there are damn few things that I think should be "cast in stone". Too often, following rules leads to crappy and/or clumsy code as people strive for (or are *forced* into) "compliance at all costs".

My coding standards are titled "Coding GUIDELINES". They're goal is to force as little as possible on the code. Instead, they try to point out the hazzards associated with various coding practices that might not be obvious to the casual coder. They try to shed light on many of the "implementation defined" behaviors of compilers (which many coders seem to be ignorant of). They try to show how certain ways of expressing algorithms can lead to unexpected consequences in different environments/tool chains/etc.

As to "understandability", you can't (realistically) quantify that. But, you know it when you see it. :>

OTOH, you also know when something is unclear and/or prone to misunderstanding (e.g., if you're counting parenthesis, you've probably got an expression that could benefit from reduction).

In case you failed to see my point:

void user_interface(void) { while (FOREVER) { event_t event = get_event(source); (void) process_event(event); } }