Chaining IF Statements in Single Positive Situations

[sanityimpaired]

There are many places within MaNGOS code in which we have an ID to compare against a list, typically using bit masks. Only one entry in the list is valid.

Rather than using a series of else if statements so that the fucntion stops once it hits a positive, I am often seeing a chain of if statements, forcing the function to keep comparing bitmasks needlessly. A fantastic example of this is EffectDummy() in SpellEffects.cpp. Some clever use of case statements makes it less tedious, but even then the case statements usually run in addition to if statements.

This seems to be fairly consistent, and it puzzles me. Wouldn't it be more efficient to use else if statements for every bit mask comparison after the first? Even better, if there's a case statement as well, wouldn't we be better off putting a chain of else if statements in the default case?

[balrok]

edit: ahh i think i missunderstood you - you're complaining about:

a=5;

if(a==4)

//do this

if(a==5)

//do that

if(a==6)

//unneeded checks better use else

--

if you mean this, i think an elseif will be better - and it was just bad codestyle (don't think mangoscode is perfect )

i think c++ compares are lazy - so:

if(false && true && true && someimportantfunction())

the important function and all true won't be evaluated

cause the first false made the whole comparision false

also i think

if(false || true || function())

won't evaluate the function too

cause i wasn't sure and it's good to know i made this program

bool func(int x)
{
   std::cout << x;
   std::cout << "\\n";
   return x<5;
}

int main()
{

   if(func(10) && func(2))
       std::cout << "pass\\n";
   if(func(3) || func(11))
       std::cout << "pass\\n";
   if(false && func(4))
       std::cout << "pass\\n";
   if(true || func(7))
       std::cout << "pass\\n";
   return 1;

the output was:

10

3

pass

pass

thats also why it's possible to do

BattleGround* bg=plr->GetBattleGround();

if(bg && bg->isArena())

it stops if bg is a NULL-pointer - if it would continue after the NULLpointer the server would crash.. but it won't continue after this..

[freghar]

...

In fact, many compilers will do some optimizations, unless you directly prohibit that, so if you use two printf() calls one after another, they'll be most likely merged in the final code to one write().

So those two snippets will probably get compiled in the same way:

if (one) {
   if (two) {
       do_stuff();
   }
}

if (one && two) {
   do_stuff();
}

Conditions which are always true will be mostly ignored (with a warning)

if (1) {
   do_stuff();
}

and so on ..

So it's just about code style

[balrok]

i think the question was, why things like:

if(a==4)
   do(1);
if(a==5)
    do(2);
if(a==6)
   do(3);

are used instead of elseif's (if it's clear that a won't change)

i guess the compiler can optimize such things until a certain level too

but i also could imagine things like this:

m_global=1;
if(m_global==1)
   changemglobal(3);
if(m_global==2)
   do(3);

i think optimizing those if's wont be so easy for the compiler, cause he needs to look for every functioncall inside this if

i think it's better to use "else if" in those parts, it's also better for the reader, cause it makes the code more understandable (cause it's more logic then)

(but those are all just my thoughts - and i'm not very skilled with c++/compiler yet)

[sanityimpaired]

That's exactly whatI mean, Balrok. After a little more digging, I noticed that many of these if statements actually use a return to end the function entirely, so the next if statement doesn't really matter. Which changes my question a bit. Consider the following snippet from EffectDummy() in SpellEffects.cpp, with most of the code cut out.

switch(m_spellInfo->SpellFamilyName)
{
       ...

       case SPELLFAMILY_WARRIOR:
           // Charge
           if(m_spellInfo->SpellFamilyFlags & 0x1 && m_spellInfo->SpellVisual[0] == 867)
               ...
               return;
           // Execute
           if(m_spellInfo->SpellFamilyFlags & 0x20000000)
               ...
               return;
           // Slam
           if(m_spellInfo->SpellFamilyFlags & 0x0000000000200000LL)
               ...
               return;
           switch(m_spellInfo->Id)
           {
               // Warrior's Wrath
               case 21977:
                   ...
                   return;
               // Last Stand
               case 12975:
                   ...
                   return;
               // Bloodthirst
               case 23881:
                   ...
                   return;
           }

           ...

           break;
}

So it works from an efficiency standpoint, but we've got multiple exit points everywhere, which goes against everything I've been taught about functional design. This whole thing could be done so much more elegantly as follows:

switch(m_spellInfo->SpellFamilyName)
{
       ...

       case SPELLFAMILY_WARRIOR:
           switch(m_spellInfo->Id)
           {
               // Warrior's Wrath
               case 21977:
                   ...
                   break;
               // Last Stand
               case 12975:
                   ...
                   break;
               // Bloodthirst
               case 23881:
                   ...
                   break;
               default:
                   // Charge
                   if(m_spellInfo->SpellFamilyFlags & 0x1 && m_spellInfo->SpellVisual[0] == 867)
                       ...
                   // Execute
                   else if(m_spellInfo->SpellFamilyFlags & 0x20000000)
                       ...
                   // Slam
                   else if(m_spellInfo->SpellFamilyFlags & 0x0000000000200000LL)
                       ...
           }

           ...

           break;
}

So why are we doing the former rather than the latter?

[VladimirMangos]

return;

All this "not chained" ifs have code ends by "return" so no repeating checks in success case.

In fail check case original and "else if" version do same checks...

But i agree that for speed _maybe_ switch(m_spellInfo->Id) batter have first before ifs.

BUT this also dependent how often different spells casted...

[freghar]

Yes, you may know a lot of things about algorithms, pascal-like code style without breaks, fortran, etc., but, c'mon, those are oldies.

Imagine a function where you want to check if certain things are met before going further and you want those checks to be easily removable / changeable without much rewriting.

int handle_interface(struct skbuff *skb)
{
   // does the sk_buff struct exist?
   if (!skb || !skb->len)
       return -1;

   // does it have a valid input interface?
   if (!skb->dev)
       return -1;

   // make a copy and sort it in an array
   if (!skb_make_writable(skb))
       return -ENOMEM;

   ......
}

Got the idea? It's about logic sorting of checks so you can easily debug each part and easily comment out to make tests and such stuff. Not to mention errno setting ..

[VladimirMangos]

Yes, this is filter way of function code organization: at first checks and special cases with fast return then normal way work code.

I like also this way code organization: easy reable without 2 page in right block indents. And easy edited later as pointed already.

[sanityimpaired]

@Vladimir: I noticed the returns for my second post, which is why I changed my question.

@Freghar: In the example I gave, the type of organization you're describing isn't necessary. I can see how doing this makes code more modular, you can move pieces of it around and comment large pieces out without needing to redo any other code. It still rubs me the wrong way, though; I had it pounded into my head both in academia and in the workplace that multiple returns like this are only acceptible if the language doesn't support any other method - and in this case it does.

Just to be clear, I'm not trying to be argumentative, I'm just having difficulty comparing what I'm seeing to how I've been trained.

[VladimirMangos]

academia and in the workplace that multiple returns like this are only acceptible if the language doesn't support any other method

I can't agree with like statement. Yes, proved that algorithm can be writed using only if-then-else, loop-with-single-exist and block. But in case loops already show that in some cases not allowing to break in loop in many points make need use additional state flags for checks and make code lot less clean. You must understand when theaching programming like pedantic way learn programmer use structured code and avoid avil GOTO Truth as oftent in middle. Also note when structural programming pedantic rules writed programming languages mostly not have exceptions support and nothing break linear code execution controlled by if/loop.

Filter write way of function write is more like how exception code work: test and catch exceptions, then execute if all ok normal defult code. Noone say that all code must be in writed in way

if(a) { s1; return;} if(b) return; ... s2;

But this useful when a,b is exception/secial cases that need proccessed in special way.

when use if(a) s1; else s2; then expected from code that s1 and s2 are _both_ normal case;

when used if(a) { s1; return } s2; then s1 expected as special case and this very helpful see this from code structure.

[freghar]

You must understand when theaching programming like pedantic way learn programmer use structured code and avoid avil GOTO Truth as oftent in middle. Also note when structural programming pedantic rules writed programming languages mostly not have exceptions support and nothing break linear code execution controlled by if/loop.

Yes, "goto" came on mind mind here as well, in fact I can still see many teachers saying that gotos are evil just because it was evil 50 years ago in pascal (ie. short label names). They're in fact even useful when the programmer knows where to use them and doesn't make them the main feature of a language.

but i also could imagine things like this:

m_global=1;
if(m_global==1)
   changemglobal(3);
if(m_global==2)
   do(3);

i think optimizing those if's wont be so easy for the compiler, cause he needs to look for every functioncall inside this if

This code won't ever get optimized, not even with -O3. It's because the compiler has some sanity checks and can decide when it's safe to assume a true statement and where the memory corruption may have serious impact. Moreover, it can't know if the function won't change its work during execution.

To get out of my "offtopic" - I personally agree on using "else if" when we're doing some checks which have something in common and switch can't be used, just like the topic author's example above. Just wanted to say that standalone conditions without "else" have some usage as well.

[sanityimpaired]

So the problem here is that a segment of the industry is pushing concepts which had a valid point historically but are no longer pertinent. I suspect this would be compounded by the fact that my employer was very tightly in bed with my academic institution; they recruited on-campus and hired a large portion of my graduating class.

That gives me a great deal to think about. Thank you both!