Logical genius!

Simone Serponi

I found this pearl while doing a peer code review. This is not the actual code but it gave the "idea"...

typedef struct
{
// Lot of stuffs..
bool m_bUsed;
} CONFIG;

#define MAX_CFG 10
static CONFIG g_vConfigurations[MAX_CFG];
...

bool IsUsed ( int iHandle )
{
if ( g_vConfigurations[iHandle ].m_bUsed == true )
return true;
else
return false;
}

A logic genius... And notice the lack of argument value check before using iHandle in IsUsed() function ;P .

Bye By(t)e ;-)

Dr Walt Fair PE

Yeah, I've done that before, then looked back at my own code and said: WTF?

CQ de W5ALT

Walt Fair, Jr., P. E. Comport Computing Specializing in Technical Engineering Software

Chris Boden

That's pretty much the reaction I have to any code I write.

supercat9

I don't really mind code of the form

if (condition)
return 1;
else
return 0;

While it certainly could be replaced by:

return (condition) != 0;

the first form makes clearer that what's being handled isn't just data, but control information. I don't like comparisons with 'true', though, unless there's a clear and explicit reason, which should be commented (e.g. 'If "condition" has an invalid value, it got glitched, so return zero to show failure.').

Lost User

supercat9 wrote:

While it certainly could be replaced by: return (condition) != 0;

Why the extra condition, what's wrong with:

return condition;

It's time for a new signature.

supercat9

Richard MacCutchan wrote:

Why the extra condition, what's wrong with:

The statement "return (condition) != 0;" is guaranteed to return either a one or a zero, regardless of the value of condition. If condition holds a value other than one or zero (whether or not it ever should), the statement return condition; would return that value. Also, I forgot to mention a few more advantages of the "if" form:

Although the posted skeletal example didn't include any comments, one may readily add a comment to distinguish the success and failure cases.
The 'if' form is readily adaptable and remains readable regardless of whether condition code meanings in the called and calling functions are the same or opposite.
Code and breakpoints may be added to the error- or normal-behavior case without changing the program structure.

Some people may make fun of the 'if' form, but I tend to like it.

Stanciu Vlad

Simone Serponi wrote:

And notice the lack of argument value check before using iHandle in IsUsed() function

Who needs to check the argument for a valid range? :confused: Hey, it's not my fault if you don't know how to properly use a function :laugh:

I have no smart signature yet...

Lost User

Agreed; I merely assumed that condition is a boolean expression that will always return true `or` false - which is, of course, what it should do.

It's time for a new signature.

scotchfaster

It looks to me like the author knew that they were writing bad code, and tried to hide the fact that they were using globals, which just made it all that much worse. But we all had to start somewhere. Slapping a poorly written function over the global accessor could be an evolutionary step towards a static class interface that properly encapsulates the data.

Simone Serponi

Yes, as you said, the code snippet I've posted is not complete (As I told I've not posted the actual code) However the coder was NOT minding to use the 'if' form to ensure return 'true' or 'false' independently from what value is in m_bUsed (moreover this is guarantee to be 'ture' or 'false' only by the code that modify it). A more rational implementation using the 'if; form could be the following:

bool IsUsed ( int iHandle )
{
if ( iHandle < MAX_CFG )
return ( true == g_vConfigurations[iHandle ].m_bUsed );
else // iHandle >= MAX_CFG
return false;
}

or (in a more 'compact' form someone can like):

bool IsUsed ( int iHandle )
{
retrun ( iHandle < MAX_CFG )? ( true == g_vConfigurations[iHandle ].m_bUsed ) : false;
}

Someone can complain about the logic of returning false when parameter is out of range instead of an exception but we can use them (that is a portion of code that recently ported to C++ from C using an "incremental" approach) and stating that "you cannot be using something you don't have" is not so wrong ;P

Bye By(t)e ;-)

Simone Serponi

That is exactly what he said when I've told him to add the argument check :laugh: .

Bye By(t)e ;-)

cgh1977

I know this is an old thread, but being the pedant I am I can't resist. None of the examples of the 'right' way to do it are optimal. Remember that m_bUsed is already defined to be a bool. Also notice iHandle is an int, which could be negative.

typedef struct
{
// Lot of stuffs..
bool m_bUsed;
} CONFIG;

#define MAX_CFG 10
static CONFIG g_vConfigurations[MAX_CFG];
...

bool IsUsed ( int iHandle )
{
if (iHandle >= 0 && iHandle < MAX_CFG)
return g_vConfigurations[iHandle ].m_bUsed;
return false;
}