Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Ask HN: Can someone explain why this line exists?

news.ycombinator.com

Post reply on HN
Print view View original

11–17 of 17 posts

Re: Ask HN: Can someone explain why this line exists?

#11

Post by ollysb »

Dylan16807 wrote: post #10
ollysb wrote: post #9

Earlier quoted context omitted.

You could express the same thing without comments: boolean alreadyLogged = !_ok; boolean skipLogging = alreadyLogged && (Math.random() > 0.1) if( skipLogging ) return res;

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() But if it's a choice between comments and adding tempor…

I wouldn't actually have them as Boolean variables, I'd extract them into query methods, then the code would just be:

    if( skipLogging() ) return res;

Re: Ask HN: Can someone explain why this line exists?

#12

Post by qu4z-2 »

Dylan16807 wrote: post #10
ollysb wrote: post #9

Earlier quoted context omitted.

You could express the same thing without comments: boolean alreadyLogged = !_ok; boolean skipLogging = alreadyLogged && (Math.random() > 0.1) if( skipLogging ) return res;

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() But if it's a choice between comments and adding tempor…

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".

Re: Ask HN: Can someone explain why this line exists?

#13

Post by zaroth »

avibryant wrote: post #8
andrewgross wrote: post #3

Earlier quoted context omitted.

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.

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() Wait, what?

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

Re: Ask HN: Can someone explain why this line exists?

#14

Post by ranman »

qu4z-2 wrote: post #12
Dylan16807 wrote: post #10

Earlier quoted context omitted.

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() But if it's a choice between comments and adding tempor…

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".

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.

Re: Ask HN: Can someone explain why this line exists?

#15

Post by 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!"

Re: Ask HN: Can someone explain why this line exists?

#16

Post by lmm »

ranman wrote: post #14
qu4z-2 wrote: post #12

Earlier quoted context omitted.

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".

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.

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.

Re: Ask HN: Can someone explain why this line exists?

#17

Post by Dylan16807 »

ranman wrote: post #14
qu4z-2 wrote: post #12

Earlier quoted context omitted.

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".

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.

Register renaming is one of the most very basic levels of optimization. I challenge you to find me a single optimizing compiler that slows down when you give names to temporary results. Hell, I can show you non-optimizing compilers that suffer no slowdown from code like this.
Post reply on HN