

Ask HN: Can someone explain why this line exists? - andrewgross

https://github.com/mongodb/mongo-java-driver/blob/master/src/main/com/mongodb/ConnectionStatus.java#L213<p>Found via: https://twitter.com/avibryant/status/339840598871769088
======
dampier
If you read the code, you will notice that the FIRST time this error is
encountered, it will always be logged.

The test in question is done frequently, and subsequent failures are about 100
times less interesting than the first. Unless you just like filling up disks
with log files while your network is acting up.

Finally, note that the "_ok" flag will be RESET and the resolved condition
logged once the test finally returns to succeeding.

~~~
andrewgross
Awesome, thanks for the explanation

~~~
lotyrin
Thank you for an excellent counterexample for when people claim code comments
are useless.

~~~
ollysb
You could express the same thing without comments:

    
    
        boolean alreadyLogged = !_ok;
        boolean skipLogging = alreadyLogged && (Math.random() > 0.1)
        if( skipLogging ) return res;

~~~
Dylan16807
I strongly dislike your version. Creating a boolean just to use and discard in
the next line? Twice? Ugly. I'd much rather keep the code simple and try to
cut out confusing parts. Give the flag a better name and remove the useless
ternary. I'd also get rid of the conditional return and let the one outside
the catch do the work.

    
    
      if (!errorLogged || (Math.random() < 0.1)) {
        
        errorLogged = true;
        
        // log error
        
      }
    

But if it's a choice between comments and adding temporary variables that
don't do anything, I'll almost always choose comments.

~~~
qu4z-2
I'd tend to choose temporary variables. I trust my compiler to optimise them
out, and I find it makes it much easier to find the appropriate place to
change or add new code.

"Does this affect working out whether the error has been logged before?",
"Does this affect whether we should log this error?", etc

EDIT: But in this case I'd still want to put a comment clarifying the "why".

~~~
ranman
are you sure the compiler is smart enough to do that? I think we frequently
assume compilers are smarter than they really are because we don't fully
understand them anymore... but in practice I find they often do very dumb
things.

~~~
lmm
No, but I am sure that my automated performance testing procedures will detect
if I've introduced a significant performance problem, and it would be an
inefficient use of my time to worry about insignificant performance effects.

------
zaroth
I commented earlier, but I can't get over how ridiculous that line is. If you
started by just writing the intent (!ok && rand() > .1) you actually have to
work really hard to turn it into what you have there, and introduce a bug in
the process.

Reminds me of this: <http://www.osnews.com/story/19266/WTFs_m> \- "Dude, WTF!"

------
byoung2
If an exception was thrown, and _ok evaluates to false, then approximately 10%
of the time, log the exception to the error log. Probably just to cut down on
noise in the logs, or to account for brief periods of latency where the server
appears down, but isn't.

~~~
andrewgross
Thanks, was reviewing it with a coworker and crept up on the same conclusion.
Seems a bit hard to test and definitely alarming at first glance.

~~~
avibryant
I think that you're right about the intent of the author, but unless I'm
mistaken, that's not what the code does. If _ok is false, it boils down to
if(!(Math.random() > 0.1))) which is the same as if(Math.random() <= 0.1),
which means it will return early only 10% of the time, so the exception gets
logged 90% of the time.

Wait, what?

~~~
zaroth
Upvote to you, and downvote to whoever wrote that line of code.

A ternary inside an 'if' statement, really? And even more so when one of the
return values is 'true'?

Relax, it's just a bug, right? But it would be nice if programmers practiced
basic Boolean reduction in their 'if' statements...

(!((_ok) ? true : (Math.random() > 0.1)))

!((_ok) ? true : (Math.random() > 0.1))

_ok ? false : !(Math.random() > 0.1)

!_ok && Math.random() <= 0.1

