When plowing through other peoples source i often find myself yelling and screaming that things are a complete chaos and mostly meaningless. While this is obviously partially true, it's definitely not true for the person that wrote the code originally as he/she have it all mapped out in their heads.
[goes to refill his drink]
One particular case where my minds says "change this" is mappings embedded in control logic. Case:
/* function was defined 100 lines above */
if( Type == Banana )
{
/* do Banana stuff */
}
else if( Type == Orange )
{
/* do Orange stuff */
}
else if( Type == Potato )
{
/* do Potato stuff */
}
else
{
/* issue "unsupported type" error and bail */
}
While the purpose might be perfectly understandable with real-world code, the reader has the problem that execution isn't directly stopped when the right type is found. This leads to two things: Execution of (minor) irrelevant code, and possible ambiguity in which if/else statements have been executed. (the branches might not be mutually exclusive).
Anyways, my strategy is to resolution in a separate function and keep execution in the control logic completely linear:
/* function was defined 100 lines above */
bool bOk = HandleVegetable( Type );
if( !bOk )
{
/* return error code */
}
with
bool HandleVegetable( VegetableType Type )
{
if( Type == Banana )
{
/* do Banana stuff */
return true;
}
if( Type == Orange )
{
/* do Orange stuff */
return true;
}
if( Type == Potato )
{
/* do Potato stuff */
return true;
}
return false;
}
in this way we're guaranteed type vs. execution uniqueness and as a consequence, a lot less headache in finding out which execution paths have been touched.
or i'm a newb.
Yeah, I can often hear the screams from my room.
SvarSletAbout the point you are making I have another idea. Requires more coding but is most readable to me and is also even faster than the if-else-if constructs. Have a separate function to handle each vegetable type (yes, including bananas and oranges!). Have a constant static array of function pointers to those and choose which one to call by using the Type as an index in the array.
something like:
enum Type
{
Banana,
Orange,
Potato,
TYPE_MAX
};
typedef bool (*VegetableHandlerPtr)(void);
// couldn't you choose fruit? it's a shorter word to type...
// I always need to check the c++ syntax for typedefing function pointers but i'm too lazy now :P
VegetableHandlerPtr HandlesOfVegetable[Type::MAX_TYPE]; // initialize this with the function pointers to the following functions:
bool HandleBanana()
{
/* do Banana stuff */
}
bool HandleOrange()
{
/* do Orange stuff */
}
bool HandlePotato()
{
/* do potato stuff */
}
later:
bool bOk = HandlesOfVegetable[Type]();
if( !bOk )
{
/* return error code */
}
hi marya
SvarSleti see your point. It's definitely nice to get rid of the overhead involved in searching for the right code to execute. It also nice to mix uses of Type as little as possible.
There are some fallback though. The ordering of the members Type have to be strictly defined and synchronized between the definition of Type and the initialization of the dispatch table. This could be a source of errors.
Another touchy-feely thing is if the implementing functions needs access to private data, or private functions, in the class that uses Type. Then we'd have some more coding overhead with pointers-to-member-functions, but that's doable: http://www.parashift.com/c++-faq-lite/pointers-to-members.html#faq-33.7
I'll see if i can find a good way the use the dispatch approach.
I think the preferable way would be to extract the fruit specific data through polymorphism if possible, and get rid of all the ifs and switches. If you cannot possible do it via polymorphism a switch over an enum seems a very prudent solution. The execution path is very well defined, performance is still faster than a bunch of if/elses and you can easily define error reporting if new unhandled types appear.
SvarSletAnd of course, you should always put everything in it's own function for readability. :)