Coding style for conditionals

[mzdude]

I have no interest in starting a religious war on the subject, but
I've
been doing some code reviews and running across code like

Method 1:
int x;
if( 0 == ( x = MyFunct() ) )
// do something

I personally don't like the style. I seem to remember there were
reasons not to do this but
today google is not my friend. I can't find any reasons against this.
Are there valid reasons
not to do this style?

I would prefer

Method 2:
const int x = MyFunct();
if( 0 == x )
// do something

reasons for method 2
1) declare and initialize
2) allows variable to be const

Are they any valid reasons to not do method 1?

 

[Victor Bazarov]

I have no interest in starting a religious war on the subject, but
I've
been doing some code reviews and running across code like

Method 1:
int x;
if( 0 == ( x = MyFunct() ) )
// do something

I personally don't like the style. I seem to remember there were
reasons not to do this but
today google is not my friend. I can't find any reasons against this.
Are there valid reasons
not to do this style?

Doesn't "pronounce" well. I can't come up with a good "spelling" of
this when I, say, talk to somebody on the phone.

I would prefer

Method 2:
const int x = MyFunct();
if( 0 == x )
// do something

reasons for method 2
1) declare and initialize
2) allows variable to be const

Are they any valid reasons to not do method 1?

They are valid.

I would prefer

... if (x == 0)

but I read/heard all arguments for your way (no need to repeat those,
thank you).

Aside from satisfying someone's whim about writing in C style, I don't
see any reason to define a variable without initializing it (in a case
like yours).

V

 

[Stefan Ram]

int x;
if( 0 == ( x = MyFunct() ) )

const int x = MyFunct();
if( 0 == x )

I removed the disturbing »0 == «

int x;
if( !( x = MyFunct() ) )

const int x = MyFunct();
if( !x )

and the disturbing space

int x;
if( !( x = MyFunct() ) )

const int x = MyFunct();
if( !x )

. The second example is better, because of the »const«.

 

[Joshua Maurice]

I have no interest in starting a religious war on the subject,

That's good. For questions of style, I think that there are two kinds
of questions:
1- entirely subjective differences, and
2- differences that result in easier to read, easier to maintain,
easier to write and prove correct, code. These differences have
objective measures in principle, though in practice it's often quite
hard to measure.

Note that there is a a very fuzzy line between these two given that
evidence in practice is so hard to obtain.

but I've
been doing some code reviews and running across code like

Method 1:
int x;
if(  0 == ( x = MyFunct() ) )
    //  do something

I personally don't like the style. I seem to remember there were
reasons not to do this but
today google is not my friend. I can't find any reasons against this.
Are there valid reasons
not to do this style?

If the variable x is used nowhere else, I would argue against this
style. I accept that sometimes breaking an expression or statement
into multiple statements can increase readability, but I see
absolutely no gain of readability of
int x;
if( 0 == ( x = MyFunct() ) )
// do something
over
if (0 == MyFunct())
// do something
I would argue that there is an objective measure in this case. We
could make a test for how fast and how accurately people can
understand the two code samples, and I argue that the evidence would
indicate that most people overall find the second easier to
understand, and they would take less time to understand it.

I would also argue that there exists an objective measure in this case
that it takes less time to read the shorter code sample, and that
there are less errors overall in this style (from having less
extraneous visible variables).

I would prefer

Method 2:
const int x = MyFunct();
if( 0 == x )
        // do something

reasons for method 2
1) declare and initialize
2) allows variable to be const

Are they any valid reasons to not do method 1?

I agree that this is preferable under the same aforementioned
objective measures, but again, if x is unused elsewhere, then it
really should be:
if (0 == MyFunct())
// do something

Of course, if MyFunct() is not exactly as written, such as if it
contains a lot of arguments or something, then that might be good
reason to separate it out into separate pieces to be more quickly and
easily understandable. I'm trying to come from the perspective of
several objective measures of good code, but as I mentioned such
discussions are usually terribly nuanced, and often overall with
simply personal preferences apart from objective measures of code
quality.

 

[MikeWhy]

I have no interest in starting a religious war on the subject, but
I've
been doing some code reviews and running across code like

Method 1:
int x;
if( 0 == ( x = MyFunct() ) )
// do something

I personally don't like the style. I seem to remember there were
reasons not to do this but
today google is not my friend. I can't find any reasons against this.
Are there valid reasons
not to do this style?

I would prefer

Method 2:
const int x = MyFunct();
if( 0 == x )
// do something

reasons for method 2
1) declare and initialize
2) allows variable to be const

Are they any valid reasons to not do method 1?

A not so tenuous reason is return types other than int, possibly involving
non-trivial default construction, or reference types.

const Foo & YourFunct();
....
const Foo & bar = YourFunct();
if (!!bar)
{ // do stuff
}

//------ or,
Foo baz(YourFunct());
if (!baz)
{} // intentionally blank
else
{ // do non-const stuff.
}
//==========

As you can see, consistent stylistic rules still leave too much room
individual deviation.

As a stylistic concern, I'll often insist on a bool operator!() const in the
class rather than an operator bool() const. This leads to the bang-bang
test, which in turn leads repetitively to answering the inevitable "what the
heck" question.

Embrace the differences, unless they're truly awful. I personally dislike
many things that I see in your code, but to change each occurrence as I
encounter them will keep us both busily unproductive for quite some time.
(All of them little things.... if( test ), rather than if (test), for
starters. Unrelated, but still topical: operator++() versus operator++(int).
I'll often define a pre-increment but not a post-increment, so code should
default to pre-increment, following STL convention, if no other compelling
reason exists.)

 

[gwowen]

The trouble with this is if you're testing for non-zero, rather than
zero, then the natural analogues are

if(x = MyFunct()) { ... }

if(!!(x = MyFunct())) { ... }

one of which is ugly, and the other of which is a maintainence
disaster waiting to happen.

const int x = MyFunct();
if(x != 0) { ... } // I'd accept if(x){ ... } here

 

[mzdude]

Thanks for the feedback. I think I'll suggest moving to
method 2 but stop short of failing the code review. Nobody
has given a concrete example of why it's bad.

 

[Michael Doubez]

That's good. For questions of style, I think that there are two kinds
of questions:
1- entirely subjective differences, and
2- differences that result in easier to read, easier to maintain,
easier to write and prove correct, code. These differences have
objective measures in principle, though in practice it's often quite
hard to measure.

Note that there is a a very fuzzy line between these two given that
evidence in practice is so hard to obtain.




If the variable x is used nowhere else, I would argue against this
style. I accept that sometimes breaking an expression or statement
into multiple statements can increase readability, but I see
absolutely no gain of readability of
    int x;
    if(  0 == ( x = MyFunct() ) )
        // do something
over
    if (0 == MyFunct())
        // do something

But you may have a gain in a core dump (or under debug); it makes it
easier to trace back the state of the program.

[snip]

I prefer separating the function call and the test (supposing the
return value is significant as Joshua Maurice pointed).
The readability of multiple = in a condition is already horrible,
method 1 makes it worse.

The only reasonable case for using method 1 is in a while loop.
while( (x=foo()) == OK )
{
// do something
}
if( x == ERROR )
{
// ... reason of loop end
}

I would also add that it is better to use braces:
if( x == 0 )
{
// do something
}

Concerning the "'constant' == variable", IMHO there used to be valid
reason to do so but nowadays compilers warn you about assignment in
conditional so I prefer the readability rather than the supposed added
security.

 

[Stefan Ram]

if(x = MyFunct()) { ... }
if(!!(x = MyFunct())) { ... }
one of which is ugly, and the other of which is a maintainence
disaster waiting to happen.

For amateur maintainers.

Which makes me think of Linus TorvaldS, who said:

»Quite frankly, even if the choice of C were to do
*nothing* but keep the C++ programmers out, that in
itself would be a huge reason to use C.«

 

[gwowen]

  For amateur maintainers.

Well, if you can guarantee that your code will be maintained in
perpetuity by dedicated professionals then lucky you.

Even so, if I saw
if(x = MyFunct()) {

}

I'd immediately think ... "do I really mean assignment"? or has
someone mistyped x==Myfunc(). In fact, if I were ever to use that
construction (probably in while() rather than if), I'd feel obliged to
comment the intent, to make the maintainers job easier.

while(cached_x = expensive_function()){ /* NB: NOT == */
// do something with cached_x
}

and even then I'd try and rewrite it to avoid that construction. It's
simply too easy to misread the '=' as '==' while trying to get a
synoptic view of the algorithm. I'm confident enough in my
intelligence that I don't need to prove it at every turn (Hi Leigh!).

"Quite frankly, even if the choice of C were to do
*nothing* but keep the C++ programmers out, that in
itself would be a huge reason to use C." -- Linus Torvalds

"Programs should be written for people to read, and only incidentally
for machines to execute." -- from "Structure and Interpretation of
Computer Programs" by Abelson and Sussman

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are, by
definition, not smart enough to debug it." -- Brian Kernighan

 

[Stefan Ram]

Programs should be written for people to read

Yes, but obviously for people who already
/know the programming language/.

 

[gwowen]

  Yes, but obviously for people who already
  /know the programming language/.

Yes, but that doesn't mean that all valid language constructs are
equally easy to maintain even for people skilled in the language ...
that's, if not demonstrably untrue, then at least widely considered
untrue.

Code that can be immediately and correctly understood at a glance is
easier to maintain than code that is a single glyph different from far-
more-common idiom with wildly different semantics.

"if(y == f()){}" is common, idiomatic C. "if(y = f()){}" is not.
Since you, and those who maintain your code, never ever make mistakes
while reading code, that won't matter. For the rest of us, clarity is
a genuine benefit.

 

[Stefan Ram]

"if(y == f()){}" is common, idiomatic C. "if(y = f()){}" is not.

To me, the meaning of both is immediately obvious.

Since you, and those who maintain your code, never ever make mistakes
while reading code, that won't matter.

I would never claim this. But anyone who took a beginner's
class in C or C++ would not err with regard to such simple
and obvious statements.

 

[gwowen]

  To me, the meaning of both is immediately obvious.

The point isn't mental, its visual. It isn't that one doesn't *KNOW*
the difference between x=f() and x==f(), its that one doesn't *SEE*
the difference between x=f() and x==f(). If your eyesight and mind
are so razor sharp that you cannot imagine mistaking one for another
while reading code, then I'm very jealous of you.

It's like this: what does following code do

int k = 8, l = 9;
int x = (k*k) - (7*l) ;
if(x = l) printf("x is one\n");
if(x == l) printf("x is one\n");

printf("%d\n",x);

 

[Puppet_Sock]

It's like this: what does following code do

int k = 8, l = 9;
int x = (k*k) - (7*l) ;
if(x = l)  printf("x is one\n");
if(x == l) printf("x is one\n");

printf("%d\n",x);

It triggers comments from me in code review.
That's what it does.
Socks

 

[Stefan Ram]

If your eyesight and mind are so razor sharp that you cannot
imagine mistaking one for another while reading code, then
I'm very jealous of you.

Indeed, I at least once mistyped »=« for »==« (while I knew
the difference), but I can not remember having misread them.

I'd compare this to English: Do you see anything special in
the next paragraph?

Inded, I at least once mistyped »=« for »==« (while I knew
the difference), but I can not remember having misread them.

How long did it take you to spot the change?

It's like this: what does following code do
int k = 8, l = 9;
int x = (k*k) - (7*l) ;
if(x = l) printf("x is one\n");
if(x == l) printf("x is one\n");
printf("%d\n",x);

Ok, I will wrap it into a compound statement (block):

{ int k = 8, l = 9;
int x = (k*k) - (7*l) ;
if(x = l) printf("x is one\n");
if(x == l) printf("x is one\n");
printf("%d\n",x); }

Next, I will reformat it slightly:

{ int k = 8, l = 9;
int x =( k * k )-( 7 * l );
if( x = l )printf( "x is one\n" );
if( x == l )printf( "x is one\n" );
printf( "%d\n", x ); }

Now I eliminate k:

{ int l = 9;
int x =( 8 * 8 )-( 7 * l );
if( x = l )printf( "x is one\n" );
if( x == l )printf( "x is one\n" );
printf( "%d\n", x ); }

Eliminate l:

{ int x =( 8 * 8 )-( 7 * 9 );
if( x = 9 )printf( "x is one\n" );
if( x == 9 )printf( "x is one\n" );
printf( "%d\n", x ); }

Evaluate constant expressions:

{ int x = 1;
if( x = 9 )printf( "x is one\n" );
if( x == 9 )printf( "x is one\n" );
printf( "%d\n", x ); }

Split the assignment into a separate statements:

{ int x = 1;
x = 9; if( x )printf( "x is one\n" );
if( x == 9 )printf( "x is one\n" );
printf( "%d\n", x ); }

Merge the initialization with the assignment:

{ int x = 9;
if( x )printf( "x is one\n" );
if( x == 9 )printf( "x is one\n" );
printf( "%d\n", x ); }

Make x const:

{ int const x = 9;
if( x )printf( "x is one\n" );
if( x == 9 )printf( "x is one\n" );
printf( "%d\n", x ); }

Eliminate x:

{ if( 9 )printf( "x is one\n" );
if( 9 == 9 )printf( "x is one\n" );
printf( "%d\n", 9 ); }

Interpret the if statements:

{ printf( "x is one\n" );
printf( "x is one\n" );
printf( "%d\n", 9 ); }

Interpret the format specifier:

{ printf( "x is one\n" );
printf( "x is one\n" );
printf( "9\n" ); }

Merge the printf statements:

{ printf( "x is one\nx is one\n9\n" ); }

Now I can unwrap the printf statement:

printf( "x is one\nx is one\n9\n" );

This is my answer to what it does.

 

[gwowen]

On Apr 7, 11:49 pm, (e-mail address removed)-berlin.de (Stefan Ram) wrote:

[snip many many lines]

  This is my answer to what it does.

Right. Because my code was really unclear. You couldn't see
immediately, what was going on - not least because on many fonts the
glyphs '1' and 'l' look very similar. The statements were all
simple, but the constructions had no clarity. It was written for a
compiler, not for a human, so it ridiculous a lot of work to untangle
it.

Inded, I at least once mistyped = for == (while I knew
the difference), but I can not remember having misread them.

Which is my point *exactly*, you write "Inded", but on a quick my mind
translates it to indeed.It is a well known truth that the human mind
sees language elements that aren't there, in order to fit the text to
established norms[0]. It's why we indent code in blocks - the
structure and logic of the algorithm is then obvious without having to
inspect every glyph. The indentation means nothing to the compiler, we
do it for the reader.

And tests in if() are a normal idiom, and assignments in if() are non-
idiomatic[0], so the brain has a tendency to see a test, even when you
wrote (and meant) and assignment.

You probably don't recall having misread them, because you've rarely
seen third-party code that uses them. It's bad style, because its
sufficiently non-idiomatic to that it looks like an error.

But hey, its just style. If you like it, be my guest, but I pity the
poor bastard who has to maintain your deliberately-unclear code. If
there's an alternative construction that isn't to tortuous, it
wouldn't pass code review here. We write for people.

[0] see http://www.chiff.com/a/Ed_Word_Recog.htm - here the if() as
the short conjunctions that allow the mind to process the wider
meaning
[1] Sufficiently non-idiomatic for gcc -Wall to warn me its a possible
error.

 

[Stefan Ram]

You probably don't recall having misread them, because you've rarely
seen third-party code that uses them.

I thought to have seen this often in the C code I used to read.
The following looks very natural and idiomatic to me:

if( f = fopen( ... ))...
if( m = malloc( ... ))...
while( *p++ = *q++ );

You might read source code from other realms than I did.

What I deem a much much larger problem for readability,
is that you can implement a bool conversation for class
instances in C++ so that

if( o )...

for a class instance o can mean anything, which has to be
looked up in the documentation of the class.

 

[gwowen]

  I thought to have seen this often in the C code I used to read.
  The following looks very natural and idiomatic to me:

if( f = fopen( ... ))...
if( m = malloc( ... ))...

Actually, those ones I do see, although usually in the negation:
if((f=fopen(...) == NULL){
// handle error
}

I'll accept those as exceptions, though because (i) fopen() and
malloc() are incredibly common, and everyone can be expected to know
how the signal failure (ii) FILE and void are (informally) opaque
types - you can't do anything to FILE* and void* directly, so return
value is essentially a bool.

Personally, since I can't declare f in the if(), and I believe in
initialisation on declaration, I'm still going to do

FILE* f = fopen();
if(f==NULL) {
// handle error
}

char* ptr = malloc(32);
if(ptr == NULL) {
// handle error
}

while( *p++ = *q++ );

<BEGIN RANT TYPE="IRRATIONAL, BORDERING ON CERTIFIABLY CRAZY">
I hate that idiom with a passion - a single statement that does seven
things, in an order implicitly dependent on operator precedence. Its
the archetypal victory of cleverness over clarity. Like obscure
technical jargon, its well understood by people who've seen it before,
but a royal pain in the arse to decipher on first exposure. Perfectly
fine in short, low level library functions - unspeakable as part of a
longer algorithm.

This is not Perl golf - unless you can demonstrate, using profiling
and disassembly, that
(i) the compiler will produce more efficient code
(ii) this efficiency actually matters to overall performance.
introducing barriers to comprehension merely to increase brevity is
premature optimisation, and premature optimisation is a very bad idea.

I want my high level code to read like pseudo-code -- the closer the
code looks is to a language-agnostic description of the algorithm, the
easier it is to see its correctness. Separating high and low level
details also makes it easier to refactor.
<END IRRATIONAL RANT>

 

[Marek Borowski]

Yes, but that doesn't mean that all valid language constructs are
equally easy to maintain even for people skilled in the language ...
that's, if not demonstrably untrue, then at least widely considered
untrue.

Code that can be immediately and correctly understood at a glance is
easier to maintain than code that is a single glyph different from far-
more-common idiom with wildly different semantics.

"if(y == f()){}" is common, idiomatic C. "if(y = f()){}" is not.

Common is "if(f()==y){}" and "if(y=f(x))". Those construction are hard
t to be mistaken. The new age programmers intro introduced
awful "if(y==f(x))" which can be mistaken with "if(y=f(x))".


Regards

Marek