Sign in to follow this  
utilae

which is better?

Recommended Posts

utilae    188
Which is better? This:
if(X=="A" && Y==1)
   ;//do stuff
else if(X=="A" && Y==2)
   ;//do stuff
else if(X=="B" && Y==1)
   ;//do stuff
else if(X=="B" && Y==2)
   ;//do stuff

Or this:
if(X=="A")
{
   if(Y==1)
      ;//do stuff
   else if(Y==2)
      ;//do stuff
}
else if(X=="B")
{
   if(Y==1)
      ;//do stuff
   else if(Y==2)
      ;//do stuff
}

Share this post


Link to post
Share on other sites
Way Walker    745
Quote:
Original post by Catafriggm
2 is better, because it has less comparisons, but if the compiler's halfway intelligent, it'll generate the same code either way.


And, because of this, you should choose whichever better describes the intent. For instance, if X can be "A", "B", or "C", then the first is probably clearer. If Y can be 1, 2, or 3, then the first is probably clearer. (If it turns out that the compiler doesn't optimize it and profiling shows that this is a bottle neck, then switch it) If X and Y are limited to the values shown, then the second is probably clearer.

Share this post


Link to post
Share on other sites
Way Walker    745
Quote:
Original post by DerAnged
#2, just looks cleaner.


Really? I like the looks of #1 better. Symmetry and all. However, #2 may be more descriptive of the logic.

By the way, if this is X is a char *, you don't want to be using == to compare strings, you want strcmp(). If X is a char, you want single quotes (') around the character. If X is a C++ stirng, never mind. But you all probably knew all that (and it's just an example).

Share this post


Link to post
Share on other sites
wendigo23    512
I say it depends on how the variables are related. Either case may present a more readable solution in different situations. If Y is like a sub-case of X, I'd go with style 2. If they are completely unrelated variables that just happen to control the same logic, I might go with style 1.

Share this post


Link to post
Share on other sites
iMalc    2466
Depending on how much there is in common between the cases, use either your option 2, or the equivalent refracted code below.
if(Y==1)
{
if(X=="A")
;//do stuff
else if(X=="B")
;//do stuff
}
else if(Y==2)
{
if(X=="A")
;//do stuff
else if(X=="B")
;//do stuff
}
Also if X can only be "A" or "B" then
else if(X=="B")
could simply be an else. Ditto goes for Y. You'd of course perhaps use an assert, to ensure it was never any other value.
The prettiness of option 1 will wear off you expand it any furthur.

Share this post


Link to post
Share on other sites
Fred304    382
Assuming X is a char and there are no other possible values than you postet, I'd go for:


switch (((X&1) << 1)|(Y&1))
{
case 3:
// do stuff (X=='A', Y==1)
break;
case 2:
// do stuff (X=='A', Y==2)
break;
case 1:
// do stuff (X=='B', Y==1)
break;
case 0:
// do stuff (X=='B', Y==2)
break;
}



You could also use an array of function pointers and call the proper function:


void doStuff3()
{
// do stuff (X=='A', Y==1)
}

void doStuff2()
{
// do stuff (X=='A', Y==2)
}

void doStuff1()
{
// do stuff (X=='B', Y==1)
}

void doStuff0()
{
// do stuff (X=='B', Y==2)
}

typedef void (*pfv)();
pfv doStuff[] = {doStuff0, doStuff1, doStuff2, doStuff3};

void test(char X, int Y)
{
doStuff[((X&1) << 1)|(Y&1)];
}



Maybe the compiler is smart enough to transfer the switch solution into something similar to the array of function pointers solution... dunno.

[Edited by - Fred304 on June 12, 2005 1:19:31 PM]

Share this post


Link to post
Share on other sites
Conner McCloud    1135
Quote:
Original post by Fred304
Maybe the compiler is smart enough to transfer the switch solution into something similar to the array of function pointers solution... dunno.

If so, I strongly suggest you get a new compiler.

CM

Share this post


Link to post
Share on other sites
Fred304    382
Quote:
Original post by Conner McCloud
Quote:
Original post by Fred304
Maybe the compiler is smart enough to transfer the switch solution into something similar to the array of function pointers solution... dunno.

If so, I strongly suggest you get a new compiler.

I meant "similar" in a way that the compiled code does not check against each of the four values and conditionally jumps but rather uses a LUT to jump to the right label. Why would that do any harm?

Share this post


Link to post
Share on other sites
Conner McCloud    1135
Quote:
Original post by Fred304
I meant "similar" in a way that the compiled code does not check against each of the four values and conditionally jumps but rather uses a LUT to jump to the right label. Why would that do any harm?

Ahh...yeah, making a jump table wouldn't be outrageous. Moving them into proper functions would not be a good move. Either way, you've completely destroyed readability with your solutions, so they're quite sub-optimal whatever the case.

CM

Share this post


Link to post
Share on other sites
Omaha    100
For small groups of comparisons like this, using an if-elseif-else structure should be more efficient than a switch. If you're working with groups of values that are relatively tightly grouped (I.e. if(x==4 || x==5 || x==6 || ...)) use a switch because then the compiler can probably just generate a jump table. And as the number of potential comparisons (I.e. if you're comparing X and Y against 20 different values) definitely use a switch.

As for which of the original two examples you should use, I concur with the others who say the second. Because if X!="A", precluding optimization, in the first case you get two failed branches and then possibly one more depending on the value of Y. In the second calse, if X!="A", you get one failed branch and then possibly one more, again, dependent on Y. But then again what is it with the SPARC you can actually tell it if the branch is expected to fail and thus that will make it more efficient if it fails... branch delay slots muck it up because then I think it actually becomes GOOD to fall through a branch or something; I can never remember exactly which architecture does what but in this case I'd go with the latter but this is really "Trying to get blood from the rock" optimization...

Share this post


Link to post
Share on other sites
Genjix    100
Quote:
Original post by stroma
second one. but if it grows up i would use switch function for them, if, elif... doesnt looks good


can't use switches on std::string's [smile]

Share this post


Link to post
Share on other sites
supercoder74    154
Quote:
Original post by Genjix
Quote:
Original post by stroma
second one. but if it grows up i would use switch function for them, if, elif... doesnt looks good


can't use switches on std::string's [smile]


use the .c_str() function to convert it to a char*.
string s;
s.c_str();//returns char* version of s

Share this post


Link to post
Share on other sites
Guest Anonymous Poster   
Guest Anonymous Poster
Quote:
Original post by supercoder74
Quote:
Original post by Genjix
Quote:
Original post by stroma
second one. but if it grows up i would use switch function for them, if, elif... doesnt looks good


can't use switches on std::string's [smile]


use the .c_str() function to convert it to a char*.
string s;
s.c_str();//returns char* version of s


Switching on char *'s is a bad idea because it probably won't do what you want. Actually, I'm not positive it even works. If it does work, then:

#include <stdio.h>
int main(void) {
char str[] = "A";
switch (str) {
case "A": puts("A"); break;
default: puts("Not A"); break;
}
return 0;
}


Will print "Not A".

If you want to do this with strings, you really need an if/else ladder, a map, or something of that sort.

Share this post


Link to post
Share on other sites
Omaha    100
Quote:
Switching on char *'s is a bad idea because it probably won't do what you want. Actually, I'm not positive it even works. If it does work, then:
*** Source Snippet Removed ***
Will print "Not A".

If you want to do this with strings, you really need an if/else ladder, a map, or something of that sort.


Actually, the compiler, seeing that all three are the same and knowing that that data will be stored in static memory may very well in that instance have "A"="A";

Everyone loves the unpredictability of compilation!

Seriously though, just avoid the whole thing and use strcmp or one of your own functions or something depending on the usage.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster   
Guest Anonymous Poster
Quote:
Original post by Omaha
Quote:
Switching on char *'s is a bad idea because it probably won't do what you want. Actually, I'm not positive it even works. If it does work, then:
*** Source Snippet Removed ***
Will print "Not A".

If you want to do this with strings, you really need an if/else ladder, a map, or something of that sort.


Actually, the compiler, seeing that all three are the same and knowing that that data will be stored in static memory may very well in that instance have "A"="A";

Everyone loves the unpredictability of compilation!

Seriously though, just avoid the whole thing and use strcmp or one of your own functions or something depending on the usage.


Actually, I specifically made str its own array so that it would print "Not A" even in the presence of a clever compiler. If str had been a pointer, it may have printed "A" or "Not A" depending on how "clever" the compiler is.

As you say, though, the point is that strcmp() is the proper way to compare C-strings.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this