Fog Creek Software
Discussion Board




Smart/Funny/Stupid Code Snippets

Yesterday, while debugging I found this code:

CApp::LogOff()
{
    :

    :
    if (m_bLoggedOn)
        m_bLoggedOn = false;
   
    :

    :
}

Why would anybody want to check m_bLoggedOn to be true to set it to false? Just  m_bLoggedOn = false would be fine enough.

John
Thursday, June 12, 2003

Looks like a case of mistaken optimisation to me.

John Topley (www.johntopley.com)
Thursday, June 12, 2003

Maybe the original developer's been bitten by doing "free(pPointer)" or "delete m_pObject" without checking for NULL first, and has forced themselves into that style as a habit.

Better Than Being Unemployed...
Thursday, June 12, 2003

delete m_pObject will perform correctly with a null pointer.

Mr Jack
Thursday, June 12, 2003

"delete m_pObject will perform correctly with a null pointer."

Very true.  A double-delete if p isn't null won't.  hence if you free a pointer outside a destructor, you ought to null it imediately.  Makes for more robust code generally.

.. just done my good thingfor the day ..

i like i
Thursday, June 12, 2003

Many programmers do not really understand boolean expressions and variables. This leads to code such as:

bool b;
if (b == true) ...

and

if (x > 3)
  b = true;
else
  b = false;

Frederik Slijkerman
Thursday, June 12, 2003

Frederik Slijkerman's post : warning C4700: local variable 'b' used without having been initialized

;-)


Thursday, June 12, 2003

Regarding delete of NULL. It should be okay. It is not on all compilers so, however. Maybe the compiler was written before the standard was set or maybe they just fscked up, but I have had to work with one that doesn't work correctly.

spaceman
Thursday, June 12, 2003


Sorry Frederik, I sense where you are going with your post and submit that it is more of a case of readability than "smart" code.

I have no problem if a shop wants to make it a standard to include things like:

  bool_var = x > 3;

but I'd usually take the more readable version any day.

DingBat
Thursday, June 12, 2003

Do you think

bool_var = (x > 3);

is more readable?

John Topley (www.johntopley.com)
Thursday, June 12, 2003


Um, actually no. That was my point.

DingBat
Thursday, June 12, 2003

Hm, I'd say that b = (x > 3) should be standard idiom for any decent programmer. I don't find

isLeapYear = (daysInYear > 365);

particularly hard to read.

Frederik Slijkerman
Thursday, June 12, 2003

Frederick, you're right.  The idea of boolean expressions having a value that can be assigned to a variable seems to be a concept that some programmers have a difficult time with.

I find things like your examples in my current project, but here is an example that tops yours. Variable names changed to protect the guilty.  This is C++:

int a(0);

// Some computations to set a to something.

bool b =  ( a == 1 ) ? 1 : 0;

mackinac
Thursday, June 12, 2003

he he.
in MFC / Win 32 programming you have all these BOOLs left over from before they had a bool type in C/C++. BOOL is typedefed to "short." In a couple of rare cases may have three distinct values, usually a third for unknown, so when the bool type was added to the compiler they couldn't change what BOOL meant.

Now if you have
BOOL b= TRUE;
bool f = b;

you get a compiler error, complaining that you are converting a short to a bool and thus possibly losing data without an ugly cast to prove that's what you want. A couple of Juno programmers took to writing

if (b)
{
  f = true;
}
else
{
  f = false;
}

good lord. Later this was reduced to
f = b?true:false;

but that's just too alarming to me. Eventually we settled on

f = !!b;

Joel Spolsky
Thursday, June 12, 2003

There is something wrong with
f = (TRUE == b);

? That seems much more readable than double !s.


Thursday, June 12, 2003

I don't like the use of !!. It's non obvious why it's there. Personally, I just use either all BOOL or all bool and don't mix and match them if I can avoid it, as they are two distinctly different types.

Better Than Being Unemployed...
Thursday, June 12, 2003

f = !!b;

??

f = b;?

na
Thursday, June 12, 2003


(BB) || (!(2B))
  "shakespeare"

na
Thursday, June 12, 2003

I agree with the idea of making code readable and can get a little obsessive about it, lining assignments up in neat columns and agonizing over variable names so they'll read nicely.  Concerning the simple example given above, it is hard to imagine anything clearer than:

    int x(0);
    bool b(false);

    // ... some computations here...

    b = x > 3;

Although in real code I'd try for better variable names.  And that 3 might get replaced by a static const value.

mackinac
Thursday, June 12, 2003

Particularly for file reading/writing functions, I like to use booleans and short-circuit evaluation, assuming a basic set of bool returning file functions, it saves checking every returning value to a write call

Like (I realize the first bOK && is not necessary, but it makes the code independent of later re-ordering)

BOOL WriteData( ... )
{
BOOL bOK = TRUE ;
bOK = bOK && WriteInt( ... ) ;
bOK = bOK && WriteStr( ... ) ;
bOK = bOK && WriteStr( ... ) ;
bOK = bOK && WriteBin( ... ) ;
bOK = bOK && WriteDouble( ... ) ;
return bOK ;
}

Better yet this idea can be used to write code that both reads and writes a file format in a single function with no nasty checking (change all "Write"s to say Archive* which are set of general bool returning read/write functions

~~
I also find that having nicely named functions (or temporary variables) to evaluate boolean expressions to be used as part/all of if's can reduce duplication and make code mode readable.

S. Tanna
Thursday, June 12, 2003

This may be a nit picky distinction, but why not the even simpler:

BOOL WriteData( ... )
{
    return WriteInt( ... )
        && WriteStr( ... )
        && WriteStr( ... )
        && WriteBin( ... )
        && WriteDouble( ... ) ;
}

Other things being equal, less stuff on the page makes for easier reading.

z
Thursday, June 12, 2003

z -

I believe that your version will not evaluate subsequent function calls once one returns false. The first code snippet will always evaluate all of the function calls.

Either one may be preferred for different problems.

Devil's Advocate
Thursday, June 12, 2003

In java at least (C++ may be different),
you always want a thread to finish running by exiting a for loop, rather than calling stop or exit on it!

so if you have a thread running a constant set of instructions in the background you would write

class X implements Runnable
{
  condition=true;

public void run()
{
  while condition
  {
    ...
  }

}
}
the method he had in there would be used to terminate the loop, i.e . run would retrun after <condition> is set to false.

Daniel Shchyokin
Thursday, June 12, 2003

Devil's Advocate, you are right  about my code snippet, but wrong about S. Tanna's.

Note that, in S. Tanna's code, when any WriteXX function returns false, bOK is set to false.  In subsequent statements bOK is the first term in the boolean expression.  Since it now evaluates to false, the second term is not evaluated, due to "short-circuit evaluation" as mentioned by S. Tanna.  The results of the two forms are the same.

z
Thursday, June 12, 2003

Some time ago I found this code snippet on the source code of a reservation system used by an airliner:

...
MOV  A, B
MOV  A, B      # I'll say it again. Just in case.
...

Also, I used to have a coworker who liked to comment obvious parts of code using rock song lyrics, stuff like:

// As Red Hot Chili Peppers said "What I've got you've got to get it put it in you"
COMMIT;

Pretty dumb. He thought it was funny though.

Daniel Tío
Thursday, June 12, 2003

RPG Daniel?

Toad the Wet Sprocket
Thursday, June 12, 2003

>There is something wrong with
>f = (TRUE == b);

The problem is that you don't capture the usual semantics of BOOL this way.  Any value that's not equal to FALSE is treated the same (by logical operators and conditional expressions).  So more likely, you'd want:
f = (FALSE != b);
This came up in comp.lang.c++.moderated a while back, and I think the consensus was the "!!b" that Joel mentioned.  I don't exactly recall why this was preferred over "FALSE != b" (and I can't find the thread right now).  I find the "!!b" to be slightly more readable, but not by much.

Brian
Thursday, June 12, 2003

z:

Since I use a source level debugger, I vastly prefer S.Tanna's style of function calling (one call to a line) when I'm in similar situations.

In theory, a smart compiler should be able to compile his code into your code with speed optimizations, but when debugging unoptimized code its very nice to be able to source step between function calls.

But that might just be a personal preference.

Steven C.
Thursday, June 12, 2003

Steven C., you do have a good point.  My goal was to make the code easy to read and understand by using a structure that was both logically and visually simple.  I wouldn't put much effort in to optimizing this, let the compiler do it.

But the concern about debugger use is valid. I have run in to such problems myself and sometimes make the code a little less neat in order to make the debugger easier to use.

z
Thursday, June 12, 2003

The point of my snippet is the short-circuit, easy of debugging/reading, and writing it this way also allows you to handle other cases that sometimes occur

BOOL WriteData( ... )
{
BOOL bOK = TRUE ;
bOK = bOK && WriteInt( ... ) ;
bOK = bOK && WriteStr( ... ) ;

if ( bOK )
{
  // prepare something else here
}

bOK = bOK && WriteStr( ... ) ;
bOK = bOK && WriteBin( ... ) ;
bOK = bOK && WriteDouble( ... ) ;
return bOK ;
}


Another thing I do which goes with which is very useful if saving into your own binary format or whatever is to add extra int's (some magic numbers which are unlikely to be in the data stream) which is very useful to help you confirm you really are in the right place in the file/stream/etc when debugging a file format

Something like

bool ArchiveVerify( int& marker )
{
  if (...writing... )
{
  return WriteInt( marker ) ;
} else
{
  BOOL bOK = TRUE ;
  int nFromFile ;
  bOK = ReadInt( nFromFile ) ;
  bOK = bOK && ( nFromFile == marker ) ;
  ASSERT( nFromFile == marker ) ; // trigger debug error if I messed up file format
  return bOk ;
}
}


BOOL ArchiveData( ... )
{
BOOL bOK = TRUE ;

bOK = bOK && ArchiveVerify( eMagic1 ) ;
bOK = bOK && ArchiveInt( ... ) ;
bOK = bOK && ArchiveStr( ... ) ;
bOK = bOK && ArchiveInt( ... ) ;
bOK = bOK && ArchiveStr( ... ) ;
bOK = bOK && ArchiveVerify( eMagic2 ) ;

~~~

In Undocumented Windows (about Windows 3.x) there is an awesome (or ugly unmaintable) bit of assembler code from Windows which reads 2 ways depending on where you start reading at a byte offset.

~~~

A very old one, (but totally stupid) that stuck in my mind (I think I read it, not saw t), was some assembly said something like

db 0x723 ; RIPLVB

Can you guess what the comment means?

S. Tanna
Thursday, June 12, 2003

HEX 723 = DEC 1827

"Rest In Peace Ludwig van Beethoven" (1770-1827).

Art Metzer
Thursday, June 12, 2003

RIPLVB is from Code Complete, right?

Brian
Thursday, June 12, 2003

It may be there, but I seem to remember first hearing about it in Commodore Pet days, which would have been early 80s

Perhaps it is an apocryphal

S. Tanna
Thursday, June 12, 2003

"The problem is that you don't capture the usual semantics of BOOL this way.  Any value that's not equal to FALSE is treated the same"

Can you expand on this for those of us who are hard of thinking this morning? Are you referring to this bizarre third state that Joel was referring to?


Friday, June 13, 2003

Daniel Tio wrote:

"Also, I used to have a coworker who liked to comment obvious parts of code using rock song lyrics, stuff like:

// As Red Hot Chili Peppers said "What I've got you've got to get it put it in you"
COMMIT;

Pretty dumb. He thought it was funny though. "

You can translate this easily into :

// I'm not interested in the quality of my job as much as this song I've got running round my head

Last year I inherited a stagnant code base from a project nobody wanted to own that was going nowhere, with the intention of turning it around. You wouldn't believe the rubbish that was buried in the source code. Song lyrics, animated postboxes, smiley faces, all sorts of rubbish. Oh, and a database library with a function called "Stupid". Hmmm...

It annoys me intensely to pick through three year old in jokes from incompetent monkeys.

Better Than Being Unemployed...
Friday, June 13, 2003

>>
RIPLVB is from Code Complete, right?
<<
Yes. By amazing coincidence, I saw the question here yesterday, then last night I read through that page of Code Complete.

<Imagine Twilight Zone music here> :)

sgf
Friday, June 13, 2003

>>> "The problem is that you don't capture the usual semantics of BOOL this way.  Any value that's not equal to FALSE is treated the same"

Can you expand on this for those of us who are hard of thinking this morning? Are you referring to this bizarre third state that Joel was referring to? <<<

I didn't post that, but I'll try an explanation.

This is C++ code.  In the example given:
#DEFINE BOOL short
#DEFINE FALSE 0;
#DEFINE TRUE  1;
    bool f;
    BOOL b;
/* Some code to assign values to f and b, then: */
    f = (TRUE == b);

If we're careful and only assign 0 or 1 to b there is no problem.  But b is a short and there are various reasons why it might have some other value assigned.  In C zero values are considered false for logical tests, non-zero are true.  Suppose b gets set to -1.  Then the following happens:

    b = -1;  /* Just for example */
    f = ( TRUE == b );  /* f is now false */
    if ( b )
      some_statement;  /* b true, this executed */
    if ( f )
        some_statement2: /* f false, not executed*/

Actually this is kind of bad C++, but the discussion was about converting C to C++.

mackinac
Friday, June 13, 2003

>>>Can you expand on this for those of us who are hard of thinking this morning?

mackinac's post pretty much sums up what I was alluding to.  The crux is that (with mackinac's variable declarations)
if (b)  // executed when b = 2
is not identical to
if (b == TRUE)  // not executed when b = 2

I wasn't thinking explicity about Joel's tri-state variable, because you hopefully wouldn't try to put that into a bool variable in the first place, and you would be more careful about making sure your variables always have one of those 3 values (so you could compare it to TRUE, FALSE and OTHER).

Brian
Sunday, June 15, 2003

Thanks mackinac. I was sure that VC++ defined TRUE as being not FALSE. It isn't. Have they changed this or is my memory playing tricks on me again?


Monday, June 16, 2003

*  Recent Topics

*  Fog Creek Home