|
I found this pearl while doing a peer code review. This is not the actual code but it gave the "idea"...
typedef struct
{
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  .
Bye By(t)e 
|
|
|
|
|
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
|
|
|
|
|
That's pretty much the reaction I have to any code I write.
|
|
|
|
|
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.').
|
|
|
|
|
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.
|
|
|
|
|
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.
|
|
|
|
|
Agreed; I merely assumed that condition is a boolean expression that will always return true<code> or false - which is, of course, what it should do.
It's time for a new signature.
|
|
|
|
|
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
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
Bye By(t)e
|
|
|
|
|
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?  Hey, it's not my fault if you don't know how to properly use a function 
I have no smart signature yet...
|
|
|
|
|
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.
|
|
|
|
|
That is exactly what he said when I've told him to add the argument check .
Bye By(t)e
|
|
|
|
|
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
{
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;
}
|
|
|
|
|
BOOL IsNegative(float val) {
char buffer[10];
char* ptr;
char* strptr;
ptr = buffer;
sprintf(ptr, "%f", val);
strptr = strstr(ptr, "-");
if (strptr == NULL)
return FALSE;
if (*buffer + (strptr-ptr) == '-')
return TRUE;
return FALSE;
}
A friend posted me this when he worked in the finance sector. I recreated it from memory and I'am sure it was slightly worse than this.
|
|
|
|
|
Hahaha, I've seen things like that before. It always makes me feel good since I know that despite how poor a programmer I really am, there is someone worse out there.
|
|
|
|
|
ARopo wrote: in the finance sector
Then he should have also checked for parentheses and red ink.
|
|
|
|
|
what I really admire about this is the somewhere it was called and tested like this
if (IsNegative(flVal))
instead of
if (flVal < 0)
though you could argue that the IsNegative function makes it clear to read at the calling end but then you could write the function like this
BOOL IsNegative(float flVal) { return (flVal < 0); }
|
|
|
|
|
Doesn't this relate back to the discussion on floating point numbers and epsilon in another thread? My C is a bit rusty (and was never good to start off with: it is the language of Satan and has driven better men than me insane) but I have a feeling that if you just compare a float to 0, you may not get the answer you expect.
|
|
|
|
|
Sometime you can get odd rounding errors when dealing with high precision, but sprintf a float into a buffer and you're more likely to introduce rounding errors, %f usually defaults to 6 decimial places
|
|
|
|
|
No, most often it does not. There shouldn't be much discussion about the sign of a number. If you want to know whether it is negative, just use x < 0. That is quite different from the question "have we reached our target yet?", which does require some slack ("epsilon") to cope with rounding errors and other minor inaccuracies.
There is one exception: the result of some operation could be an extremely small number (say -1.E-500), which is mathematically negative, but can not be represented in a 64-bit floating-point number, and then gets stored as zero (or maybe "NotANumber").
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read formatted code with indentation, so please use PRE tags for code snippets.
I'm not participating in frackin' Q&A, so if you want my opinion, ask away in a real forum (or on my profile page).
|
|
|
|
|
Reminds me of my computer science class in high school lol
|
|
|
|
|
double A;
A = ...some calculations...
if (A == 1 || String.Compare(A.ToString(), "1") == 0)
At what point does A become a string and needs a string comparison 
I know the language. I've read a book. - _Madmatt
|
|
|
|
|
Mark Nischalke wrote: At what point does A become a string and needs a string comparison
When you don't understand how floating point comparisons work. I guess.
Chris Meech
I am Canadian. [heard in a local bar]
In theory there is no difference between theory and practice. In practice there is. [Yogi Berra]
|
|
|
|
|
While that code looks ludicrous and is hiding its probable intent very well, it does make some sense, although I would prefer to tackle the issue differently. He probably is looking for values of A that are either one, or pretty close to one, so close that the default numeric format just shows "1".
Here is a little test illustrating that:
double c=1;
double b=1;
for (int i=0; i<1000; i++) {
double a=c+b;
string s=a.ToString();
if (s=="1") {
log("i="+i+" b="+b+" a="+a+"="+a.ToString("E30")+" s="+s);
break;
}
b=b/2;
}
It generates this output:
i=48 b=3.5527136788005E-15 a=1=1.000000000000003600000000000000E+000 s=1
It is always risky to use equality tests on floating-point numbers. The normal approach would be something along these lines:
if ( Math.Abs(a-1) < epsilon ) ...
where epsilon would be a small positive constant, say 1.0E-15
[EDIT] fixed a little a/c confusion [/EDIT]
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read formatted code with indentation, so please use PRE tags for code snippets.
I'm not participating in frackin' Q&A, so if you want my opinion, ask away in a real forum (or on my profile page).
modified on Thursday, May 13, 2010 10:50 AM
|
|
|
|
|
Understandable. Not having context I know it's difficult to make a proper analysis. In other places there is this
if(A == 1 || A == 2 || A == 3)
which make the first example even more ludicrous. Why a double was being used for something that could only be an int is another mystery.
I know the language. I've read a book. - _Madmatt
|
|
|
|
|
When I was learning BASIC way back in high school we only used reals (and strings), and we who noticed that putting a percent sign on a variable name (e.g. A%) would make it an integer wondered why anyone would want to use an integer when reals could do so much more... 
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
