Position of test values in conditional expressions

[=?ISO-8859-1?Q?Ney_Andr=E9_de_Mello_Zunino?=]

Hello.

I have noticed, in a lot of C and C++ code, that many programmers seem
to prefer putting the test values first in conditional expressions.
I.e., they would rather write this:

if (-1 == foobar())

than this:

if (foobar() == -1)

The second form looks more natural and easier to read IMHO and is the
one I have always used. However, given the high ocurrence of the first
one, I would like to know the implications of using one way or the other.

Thank you,

 

[Jack Klein]

Hello.

I have noticed, in a lot of C and C++ code, that many programmers seem
to prefer putting the test values first in conditional expressions.
I.e., they would rather write this:

if (-1 == foobar())

than this:

if (foobar() == -1)

The second form looks more natural and easier to read IMHO and is the
one I have always used. However, given the high ocurrence of the first
one, I would like to know the implications of using one way or the other.

Thank you,

The compiler does not care, one way or the other. And except for the
case of a C++ function returning a reference to a non-const object,
there is no gain.

But when doing equality tests against constant expressions, consider
this:

int x;
// assign some value to x
if (x == 3)
// do something

Now consider what happens if the programmer commits the not uncommon
error of typing only '=' when '==' was intended. The resulting code:

if (x = 3)
// do something

....is perfectly legal, and has the effect of evaluating the constant
value assigned to x. If the constant value is 0, the test is always
false and the conditional code is always skipped. If the constant is
not 0, the test is always true and the conditional code is always
executed.

On the other hand, if you always write:

if (3 == x)

....then if you accidentally leave off the second '=' the result is a
constraint violation requiring the compiler to issue a diagnostic.

 

[E. Robert Tisdale]

I have noticed, in a lot of C and C++ code,
that many programmers seem to prefer
putting the test values first in conditional expressions.
I.e., they would rather write this:

if (-1 == foobar())

than this:

if (foobar() == -1)

The second form looks more natural and easier to read IMHO
and is the one I have always used.
However, given the high ocurrence of the first one,
I would like to know the implications of using one way or the other.

What you have noticed is a *habit* that some programmers adopt
to help them avoid a common mistake:

int x = 0;
// . . .
if (x = -1) // Error! Should have been (x == -1)

One problem with C and C++ is that
operator== is too *close* to operator=
and the programmer might mistype the assignment operator
when the comparison operator was intended.
This typographical error is difficult to spot
when debugging your code so some programmers write:

if (-1 == x) // . . .

so that if they mistype

cat main.cc

int main(int argc, char* argv[]) {
int x = 0;
// . . .
if (-1 = x);
return 0;
}

g++ -Wall -ansi -pedantic -o main main.cc

main.cc: In function `int main(int, char**)':
main.cc:4: error: non-lvalue in assignment

The get a diagnostic message from the compiler like the one above.
In your example:

cat main.cc

int foobar(void) {
return -1;
}

int main(int argc, char* argv[]) {
if (foobar() = -1);
return 0;
}

g++ -Wall -ansi -pedantic -o main main.cc

main.cc: In function `int main(int, char**)':
main.cc:6: error: non-lvalue in assignment

You get a diagnostic if you type the assignment operator
instead of the comparison operator because foobar(void)
does *not* return an lvalue.
This is probably just an example of *force-of-habit*.
Programmers who use this trick get used to seeing the constant
on the left-hand side of a comparison and feel more comfortable
writing *all* comparison expressions that way
even if they don't contain an lvalue.

 

[JKop]

Jack Klein posted:

On the other hand, if you always write:

if (3 == x)

...then if you accidentally leave off the second '=' the result is a
constraint violation requiring the compiler to issue a diagnostic.

Very clever indeed!


-JKop

 

[Dave]

What you have noticed is a *habit* that some programmers adopt
to help them avoid a common mistake:

int x = 0;
// . . .
if (x = -1) // Error! Should have been (x == -1)

It also makes the code slightly easier to read, if you're only
interested in this function and not the stuff that it calls:

if (POSS_1 == some_complicated_expression)
{
// this code is executed if something we're not interested in
// just yet evaluates to POSS_1
}

So here we're only really interested in POSS_1 and what happens when we
encounter that result. How POSS_1 is calculated isn't really important
right now.

If this reads the other way round then you have to scan to the end of
the equality test before you find the stuff you're interested in.
Semantically of course there's no difference; if A == B then B == A, but
visually it can make code easier to understand.

Dave.

 

[Andre Kostur]

Good point well put.

IMHO: I prefer to use the (foobar() == -1) form because I know my compiler
will issue a warning if I try to "if (foobar() = 1)". (Um, one of my two
compilers.... so, yeah, I know it's not a diagnostic that the compiler is
_required_ to issue, I just know that my tool _does_).

 

[Corey Murtagh]

Andre Kostur wrote:

IMHO: I prefer to use the (foobar() == -1) form because I know my compiler
will issue a warning if I try to "if (foobar() = 1)". (Um, one of my two
compilers.... so, yeah, I know it's not a diagnostic that the compiler is
_required_ to issue, I just know that my tool _does_).

If foobar() returns a reference to a non-const object then your compiler
likely /won't/ return a diagnostic unless you tell it to be /really/
anal. For example:

int some_int = 0;

int& foobar()
{
return some_int;
}

int main()
{
if (foobar() = 1)
return 1;
return 0;
}

 

[Andre Kostur]

Andre Kostur wrote:


If foobar() returns a reference to a non-const object then your
compiler likely /won't/ return a diagnostic unless you tell it to be
/really/ anal. For example:

int some_int = 0;

int& foobar()
{
return some_int;
}

int main()
{
if (foobar() = 1)
return 1;
return 0;
}

Yep... the compiler complained about it:

test.cpp: In function `int main ()':
test.cpp:10: warning: suggest parentheses around assignment used as truth
value

(Happens to be gcc v2.96, with -Wall as a compiler flag. When I get
really picky, I turn on -Werror too, to make all warnings errors....) (In
all of my makefiles, -Wall is always supplied.)

 

[William]

If you rely on this kind of trick to write correct code, it means that you
are not doing any testing, which is the worst mistake you can make.

I think any "trick" that helps ensure correct code is worthwhile. Since
I've seen a number of problems where tests would not reveal that the
intended == was replaced with =, I wouldn't rely solely on the tests
either.

How can the tests not reveal the problem? When the code that is
excuted every time as a results of the ( x = 3) or whatever doesn't
alter the final outcome. This can happen when the test is used to
bypass an operation when it would be redundant and avoiding the
overhead is worthwhile. (I saw a problem like this in a graphics
routine - sped up quite a bit once the problem was fixed. Had the
original programmer applied a trick or two it would have been
flagged and fixed months earlier.) -Wm

 

[Darrell Grainger]

I think any "trick" that helps ensure correct code is worthwhile. Since
I've seen a number of problems where tests would not reveal that the
intended == was replaced with =, I wouldn't rely solely on the tests
either.

How can the tests not reveal the problem? When the code that is
excuted every time as a results of the ( x = 3) or whatever doesn't
alter the final outcome. This can happen when the test is used to
bypass an operation when it would be redundant and avoiding the
overhead is worthwhile. (I saw a problem like this in a graphics
routine - sped up quite a bit once the problem was fixed. Had the
original programmer applied a trick or two it would have been
flagged and fixed months earlier.) -Wm

Just something to consider...

If it costs you nothing to do this or if it is something you are willing
to spend your time learning to do (rather than the company's time) I see
nothing wrong with doing it.

If your productivity is going to drop because you are trying to get into
the habit of using this trick then I would have a problem with it. I'd
consider whether buying a good lint application would be cheaper then the
lost productivity.

Additionally, I would not impose this policy on someone else. I'd suggest
it. If they didn't feel it was worth while, do I want to spend the time
(which costs the company money) arguing the point? Would it be more
economical to just buy a lint application?

By the way, my company uses a lint application that always catches when
programmers use an assignment in a conditional expression. No need for the
programmers to learn a trick to make the compiler catch it MOST of the
time when I can get an application that catches it ALL of the time.

 

[Programmer Dude]

writes:

How can the tests not reveal the problem?

Inept compiler or no use of a lint tool?

(I saw a problem like this in a graphics
routine - sped up quite a bit once the problem was fixed. Had the
original programmer applied a trick or two it would have been
flagged and fixed months earlier.) -Wm

Just as another data point, I've never experienced the error in
over two decades of experience. Of course, my compilers always
whined about it and I never use assignment in a test expression.
(So any compiler warnings about this sort of thing ARE errors.)

 

[CBFalconer]

.... snip ...

By the way, my company uses a lint application that always
catches when programmers use an assignment in a conditional
expression. No need for the programmers to learn a trick to
make the compiler catch it MOST of the time when I can get
an application that catches it ALL of the time.

But I often *want* to embed an assignement in a conditional, and
your gang of hackers should also:

void foo(int count, char *fn)
{
FILE *fp = NULL;
bar *buf;

if (!(buf = malloc(count * sizeof *buf))) err("No mem");
else if (!(fp = fopen(fn, "r")) err("can't open");
else {
/* do my thing with fp and buf */
}
if (fp) fclose(fp);
free(buf);
}

and I can concentrate on doing my thing. The wrappers are solid,
clean, and automatic. I haven't checked fclose.

 

[Gerry Quinn]

Andre Kostur wrote:


If foobar() returns a reference to a non-const object then your compiler
likely /won't/ return a diagnostic unless you tell it to be /really/
anal. For example:

They WILL complain - that is the point.

"if ( x = y )" is perfectly legitimate syntax, but it's a pattern that
most programmers rarely if ever use.

Some people probably love it for its compactness, and they should turn
the warning off. I would consider it to fall into the category of
obfuscated code, and its use would require very special circumstances
(can't think of any offhand).

- Gerry Quinn

 

[Richard Herring]

But I often *want* to embed an assignement in a conditional, and
your gang of hackers should also:

void foo(int count, char *fn)
{
FILE *fp = NULL;
bar *buf;

if (!(buf = malloc(count * sizeof *buf))) err("No mem");

else if (!(fp = fopen(fn, "r")) err("can't open");
else {
/* do my thing with fp and buf */
}
if (fp) fclose(fp);
free(buf);
}

This thread is crossposted to c.l.c++ :

void foo(int count, char const * fn)
{
std::vector<bar> buf(count);
std::ifstream is(fn);
if (!is) throw std::runtime_error("can't open");
/* do my thing with is and buf */
// no cleanup required!
}

 

[CBFalconer]

.... snip ...

"if ( x = y )" is perfectly legitimate syntax, but it's a pattern
that most programmers rarely if ever use.

Some people probably love it for its compactness, and they should
turn the warning off. I would consider it to fall into the
category of obfuscated code, and its use would require very
special circumstances (can't think of any offhand).

I gave at least one in <[email protected]> a couple of
days ago.

 

[Karl Heinz Buchegger]

They WILL complain - that is the point.

"if ( x = y )" is perfectly legitimate syntax, but it's a pattern that
most programmers rarely if ever use.

Some people probably love it for its compactness, and they should turn
the warning off. I would consider it to fall into the category of
obfuscated code, and its use would require very special circumstances
(can't think of any offhand).

It's easy to 'turn that warning off'.
Just use the result of the assignment in a conditional.
All compilers I used up to know that warn about the assignment shut up
immediatly.

if( x = y )
turns to
if( ( x = y ) != 0 )
or
if( !(x = y ) )

Personally I consider this as a service to the programmer following
me to indicate: "Don't worry. I really ment assignment"

 

[Gerry Quinn]

I gave at least one in <[email protected]> a couple of
days ago.

I would consider your example to be obfuscated code.

Your example:
<QUOTE>
void foo(int count, char *fn)
{
FILE *fp = NULL;
bar *buf;

if (!(buf = malloc(count * sizeof *buf))) err("No mem");
else if (!(fp = fopen(fn, "r")) err("can't open");
else {
/* do my thing with fp and buf */
}
if (fp) fclose(fp);
free(buf);
}
<END-QUOTE>

I don't use C anyway but if I were writing that it would look something
like the following:

void foo(int count, char* fn )
{
FILE* fp = NULL;
bar* buf;

buf = malloc( count * sizeof( *buf ) );
if ( buf == 0 )
{
err( "No mem" );
}
else
{
fp = fopen( fn, "r" );
if ( ! fp )
{
err( "can't open" );
}
else
{
/* do stuff */
fclose( fp );
}
}
free( buf );
}

I might lose the if-elses by creating a do-loop or tracking successes
with a flag, but that's another issue.

Why do you think the above is a problem? Is it just that it's too long
for cut-and-paste? Of course one virtue of C++ is that you can usually
create a class to embody such things compactly.


- Gerry Quinn

 

[Programmer Dude]

I would consider your example to be obfuscated code.

I would agree if we insert the word, "mildly". (-:

Your example:

if (!(buf = malloc(count * sizeof *buf))) err("No mem");

...if I were writing that it [...]:

buf = malloc( count * sizeof( *buf ) );
if ( buf == 0 )

Ditto!

 

[Nick Landsberg]

Gerry Quinn wrote:

[my comments at the bottom]

I would consider your example to be obfuscated code.

Your example:
<QUOTE>
void foo(int count, char *fn)
{
FILE *fp = NULL;
bar *buf;

if (!(buf = malloc(count * sizeof *buf))) err("No mem");
else if (!(fp = fopen(fn, "r")) err("can't open");
else {
/* do my thing with fp and buf */
}
if (fp) fclose(fp);
free(buf);
}
<END-QUOTE>

I don't use C anyway but if I were writing that it would look something
like the following:

void foo(int count, char* fn )
{
FILE* fp = NULL;
bar* buf;

buf = malloc( count * sizeof( *buf ) );
if ( buf == 0 )
{
err( "No mem" );
}
else
{
fp = fopen( fn, "r" );
if ( ! fp )
{
err( "can't open" );
}
else
{
/* do stuff */
fclose( fp );
}
}
free( buf );
}

I might lose the if-elses by creating a do-loop or tracking successes
with a flag, but that's another issue.

Why do you think the above is a problem? Is it just that it's too long
for cut-and-paste? Of course one virtue of C++ is that you can usually
create a class to embody such things compactly.


- Gerry Quinn

Actually, you're both right as far as I am concerned.

When first learning a language like C, I would prefer
that newbies did it much like Gerry's example.

At this stage of the game, it is much more important to
get it right than to get it compact.

As one progresses up the ladder and becomes more expert,
then one may use what I will call the "shorthand" notation
(for want of a better term) that CBFalconer does.

Experienced programmers can read/write both without difficulty.
*Inexperienced* programmers who try to use the "shorthand"
notation (just because it's "kewl"?), usually wind up with
logic errors.

Just my opinion.

NPL

 

[Andre Kostur]

I don't use C anyway but if I were writing that it would look something

like the following:

void foo(int count, char* fn )
{
FILE* fp = NULL;
bar* buf;

buf = malloc( count * sizeof( *buf ) );
if ( buf == 0 )
{
err( "No mem" );
}
else
{
fp = fopen( fn, "r" );
if ( ! fp )
{
err( "can't open" );
}
else
{
/* do stuff */
fclose( fp );
}
}
free( buf );
}

I might lose the if-elses by creating a do-loop or tracking successes
with a flag, but that's another issue.

Why do you think the above is a problem? Is it just that it's too long
for cut-and-paste? Of course one virtue of C++ is that you can usually
create a class to embody such things compactly.

As a code reviewer, I'd accuse you of inconsistent style. Why do you
explicitly check buf against 0, but implicitly check fp? (Personally I
prefer to _always_ explicitly check, and I prefer using the NULL macro to
help reinforce that I'm dealing with a pointer and not an int).