Fog Creek Software
Discussion Board




I finally found one!

After years of always writing if statements like this:

    if (5 == nMumble)

... instead of like this:

  if (nMumble == 5)

... I finally found some code that failed because someone did it the "wrong" way:

  while (pos = NULL) {
        // delete some resources
  }

Even better: I knew there was a tiny memory leak in that function, and had even glanced briefly at the loop, and *still* didn't notice the bug - which is *exactly* the rationale for this idiom!

Okay, I'm easily amused, but it's been a crappy day, and I don't know any other group that (a) gets it, and (b) cares...

Fenster
Wednesday, July 02, 2003

Compiling this with GCC gives the warning "suggest parentheses around assignment used as truth value".  Do other compilers not check for this?

Tim Evans
Wednesday, July 02, 2003

VS7 does check for this.

If the idiom is intended, one could write the following to shut it up:

if((pos= XX) == NULL)

njkayaker
Wednesday, July 02, 2003

I'm using VC6 (long story), and this line of code has been compiled about a zillion times at Warning Level 4.  Today, the gods smiled, and the warning was displayed when I built.

Why was it never displayed before?  Beats me... vagaries of the compiler.  I'm just happy it did at all...

Fenster
Wednesday, July 02, 2003

I can't stand having constants on the left side.  I fully understand why people think it is helpful, but I think it is very unnatural to write and read code using that convention.

I just do things the old fashioned way and let the compiler warn me about such things.

Mister Fancypants
Wednesday, July 02, 2003

"unnatural" ?

what does that mean?

Joel Spolsky
Wednesday, July 02, 2003

I can't stand making my code look a little strange, so that I can all but eliminate an entire class of bugs.

I thought programmers had vulcan-like logic?

--
ee

eclectic_echidna
Wednesday, July 02, 2003

There are ways to disable warnings in VC. I.e. you can use #pragma warning disable( 4706 ) to turn off this particular warining about "assignment within conditional expression".

WildTiger
Wednesday, July 02, 2003

C# always catches that error.

J.J.
Wednesday, July 02, 2003

That was a good rule a decade ago, but we've all moved onto better compilers, I presume. Well, maybe someone's still doing Minix kernel development...

Brad Wilson (dotnetguy.techieswithcats.com)
Wednesday, July 02, 2003

You're sacrificing readability to prevent a mistake the computer can catch and let you know about.

Sounds like a poor tradeoff to me.

And the horse you rode in on
Wednesday, July 02, 2003

I put constants on the left side all the time just so that other programmers ask why and I can tell them ...

constructive comment
Thursday, July 03, 2003

Why do you want programmers to ask why, so that you can tell them?
Or did you just make this statement to make us programmers ask you why, so that you can tell us?

Someone unknown
Thursday, July 03, 2003

> "unnatural" ?
> what does that mean?

To me, the line if (x == 3) reads naturally as "if x is equal to 3". My brain parses the "if x is equal to" bit first, followed by the "3".

The equivalent "if (3 == x)" is parsed by my brain as "if 3 is equal to", followed by "x". This causes minor cognitive dissonance, because my brain thinks "3 is equal to what? 3 is always 3, it doesn't change, something must be wrong here!". This results in my taking marginally longer to understand the "if" statement.

Also, the "3 == x" idiom breaks my personal "If it looks ugly, don't do it" rule. Subjectively, it *just looks wrong*, and by the sounds of it, other people here feel the same way.

Ashley
Thursday, July 03, 2003

It's just that you aren't used to looking at it this way. Actually they should teach to use this style in institutions. I also laughed the first time I saw it. But, it got into a habit after a month or so and it has helped me to catch many bugs at compile time itself.
Just my 2 cents worth :)

Ajit
Thursday, July 03, 2003

I never put the constant first in if-statement comparisons. The rationale is this: putting the constant first is meant to prevent one from typing = instead of ==, because the compiler will catch it. I fear that doing so will make me careless, and that it will bite me back sooner or later: quite often I compare two variables instead of a variable and a constant.

So I prefer to be consistently cautious, with the added benefit that the code is more readable (I'm from the school of thought who thinks that if (x == 3) is much more readable than if 3 == x)).

On a (somewhat) related note, some people recommended to use always < and never > (or the other way around) in comparisons, but I don't like that either. I prefer

if (x > 3 && x < 10)

above

if (3 < x && x < 10).

Roel Schroeven
Thursday, July 03, 2003

"The compiler will catch it" is an attitude that causes bugs, pure and simple.  And in fact, the compiler does *not* always catch it, as I have demonstrated.

Everything you do in source code should have the aim of making bugs impossible.  That includes big things like design, and little things like this.  To do any less is unthinkable.

Fenster
Thursday, July 03, 2003

Fenster,
I think that while your attitude is laudable, you are failing to see the wood for the trees. One of the things you should be doing in source code is making it harder for bugs to exist, and easier for bugs to be spotted. It is not the only thing.

Code maintainance is not just about finding bugs, it is also about being able to extend and / or refactor as necessary. Building code which is easy to maintain, extend and refactor is also a valid goal. Bugfixing is important, but it isn't everything.

regards,

treefrog
Thursday, July 03, 2003

I really dislike this convention.  Really.  Dislike.  It.  If you're going to do that, you may as well do this:

template<typename T, typename W>
inline bool compare_equals( const T& arg_1, const W& arg_2 )
{
    return( arg_1 == arg_2 );
}

//  ...

if( compare_equals( i, 0 ) )
  //  foobar

Hey, then I can put the constant on whichever side I like, and there's no chance whatsoever I might slip up.  And it's perfectly clear what's going on.

c++_conventioner
Thursday, July 03, 2003

I think every decent C developer makes the assignment-instead-of-comparison error exactly once, then from that point on forward remembers to use the correct operator. I know that's what happened to me.

Once you appreciate the value of the convention, you've already been burnt once. Once you've been burnt, you no longer need the convention. Now I'm confused.

If half of the people here think it makes the code harder to read, and the other half don't, what's does it say about the average cost in readability?

Big B
Thursday, July 03, 2003


Heh, my "me too" response is: yeah, looks unnatural to me, too. I'm not against people using it, thought.

http://dictionary.reference.com/search?q=unnatural

;-)

Leonardo Herrera
Thursday, July 03, 2003

As long as we're nitpicking, the statement

if (5 == x)

is "wrong" since you are using a magic number!

Aren't there more important topics?
Thursday, July 03, 2003

treefrog - What's the point of making code easier to maintain, at the expense of correctness?  Nothing in this idiom makes it hard to extend or refactor.  The easiest code to maintain is the code you don't have to look at, because it doesn't have a bug.

c++_conventioner - That's just silly.

Big B. - You misunderstand.  It's not a mistake, it's a typo.  Any idiot knows to use == instead of = , but sometimes fingers slip.

Aren't there more important topics? - You are correct, sir!

And finally, "harder to read" just means, "someone else's code".  ATL is "hard to read", at first.  Does that make it bad code?

Fenster
Thursday, July 03, 2003

"hard to read" does not mean "someone else's code."  It also applies to YOUR code when you have to come back and modify it two years later.

Personally I see no substantial difference between relying on the compiler issuing an error for "if (5 = x)" and relying it issuing a warning for "if (x = 5)" (especially if you have warnings as errors, which IMO is a good practice).  In both cases you are relying on the compiler to catch a typo.  Both indicate that there was a failure.  Why is one preferable to the other?

Now obviously if your compiler doesn't issue that warning, or if you want to turn that warning off for some reason, the constant on the left may be preferable.

Mike McNertney
Thursday, July 03, 2003

Fenster... you say c++_conventioner's idea is silly.  By your own argument, his idea should be the standard.  It is the only way, for example, that you can protect against "if (x=y)".  Your whole argument is that it is OK to sacrifice some readability in order to make sure the compiler throws an error for this bug, so why wouldn't you go along with the idea c++_conventioner put forth?

Mike McNertney
Thursday, July 03, 2003

Fenster:

That was my point.  But it's only silly because the logic of constant-first comparison is silly--that is, we should all move away from simple and intuitive language semantics to avoid (rare) mishaps.

Well, if we're going to do that, why not use templatized comparison?  Because it fits that end _too well_?  Why not also use operator keywords, too?  We wouldn't want to accidentally confuse && with &.

In short:  we should encourage developers to use the language mindfully and with a sharp eye for its nuances, rather than use these catch-all tricks.  Part of being an experienced developer is being extremely careful on all levels of one's practice.

c++_conventioner
Thursday, July 03, 2003

I can't believe that reading "if (-1 == x)" will throw an experienced programmer for a loop, so I definitely agree with Mike. And since I suspect that Mike and I work on the same program (if this is the same Mike) it's refreshing to know that at least one colleague won't get annoyed when I do this.

Like most programmers I read/modify code written by others all the time. Other programmers will always have different styles. Some I really like, some I'm not so fond of. But that does not mean those styles are necessarily right or wrong.

Larry Prince
Thursday, July 03, 2003

I prefer putting constants first for readability.  The constant is typically the point of the comparison so it makes sense to put it first to highlight this. 

Consider the following where the compiler check isn't even an issue (since = will fail in both cases):

if (ERROR == FunctionCall(...))
  -- versus --
if (FunctionCall(...) == ERROR)

You can get the point of the first "if" by simply reading "if (ERROR ==" but must read the entire line to understand the second "if" (sometimes needing to scroll to the right if ERROR is outside the visible area).  It's annoying to have to read whole lines (or to scroll) when trying to quickly scan through code and it's easy to misinterpret "if (FunctionCall(..." as meaning "if FunctionCall succeeded" when reading through code fast. 

I could've sworn I read about a study (or something along those lines) about this but I haven't been able to find it since.  If anyone knows what I'm talking about, please speak up!

SomeBody
Thursday, July 03, 2003

But the important thing on that line is the function call, not the constant.

Put the most important bits first.

And the horse you rode in on
Thursday, July 03, 2003

Somebody:

Well, call me anal, but I don't nest function calls inside if statements.  Or inside anything, really.  Flat structures are just more readable, IMO.  And generally, more extensible.

For instance, in that code:

if (ERROR == FunctionCall(...))

Well, what if in the future, FunctionCall( ) is modified to return some other status besides ERROR (it happens!)?  You aren't very well going to do:

if( ERROR == FunctionCall( ) )
else if( ANOTHER_STATUS == FunctionCall( ) )

Because, of course, that executes FunctionCall( ) twice.  More likely, you want to do this:

ret_val = FunctionCall( );
if( ret_val == ERROR ) // or switch( ret_val ), etc.
// ...
else if( ret_val == ANOTHER_STATUS )
// ...

In short:  I don't really buy the readability argument here, because you could improve the code a lot by eliminating the nesting altogether.

c++_conventioner
Friday, July 04, 2003

I totally agree with you that it's usually bad style to stick function calls inside the "if". 

However, I'd still argue that it's clearer to have the constant on the left.  "if (ERROR == ..." is the important piece of information.  "ret_val" is just some placeholder variable that you're comparing it against.  Why stick the superfluous piece of information (to the reader) in the middle of the important stuff?

For the record, what I had in mind with the above function is something like Win32's GetLastError:

if (ERROR_FILE_NOT_FOUND == GetLastError())
  -- versus --
if (GetLastError() == ERROR_FILE_NOT_FOUND)

In this case, GetLastError is far from the most important thing on the line, as far as readability is concerned. 

SomeBody
Friday, July 04, 2003

SomeBody:

I don't know that "ret_val" is superfluous.  It was in that example, but only because the example itself was very generic.

But in practice, the variable isn't necessarily superfluous.  eg,

HANDLE created_file_handle = 0;
created_file_handle = CreateFile( ... );

At least in this instance, the return value is extremely important--it's the handle to your newly-created file.

So shall we write:

if( INVALID_HANDLE_VALUE == created_file_handle )

versus

if( created_file_handle == INVALID_HANDLE_VALUE )

I don't know that the particular error code is more important, in this case at least, than the fact that you're testing the created file handle for validity.  Isn't _that_ what the reader wants to know?  Who cares if the handle is invalid because its value is INVALID_HANDLE_VALUE; I just want to know that the handle is OK.

c++_conventioner
Friday, July 04, 2003

It is hard to argue for legibility in a language that wasn't designed to be safe or legible.

Greg
Friday, July 04, 2003

Obviously the return value is important to the proper execution of the program.  I'm speaking solely in terms of a human viewing the code, not the compiler. 

Consider the following two examples:

data_file_handle = ...
if( data_file_handle == ... )

... CreateFile( ... );
if( INVALID_HANDLE_VALUE == ... )

It's not clear at all what the first example does, other than getting some file handle and comparing it to something.  It might be creating the file or it might be retrieving the file from a previously opened set of files.  It might be comparing against an error value or against another file handle. 

It's obvious what the second example does, even without the variable being visible.  That's what I mean about the variable being superfluous.  Conceptually, the important thing is that you are creating a file and checking for failure, not that you are assigning a variable and comparing it to something. 

SomeBody
Friday, July 04, 2003

Ok, you all are idiots.  Ok, first off.  If you don't have the capacity to double check your code then you have no business doing it. 

Pay attention to what you are doing.

ok.. then here is one for all of you!!!

int tmp;
tmp = 5;
while(tmp==5){
  printf("Shutup for the %dth time!",(tmp--)-6); 
}

l33t h4x0r
Friday, August 20, 2004

*  Recent Topics

*  Fog Creek Home