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

What's wrong with this code, really?

cvmountain.com

Post reply on HN
Print view View original

11–20 of 217 posts

Re: What's wrong with this code, really?

#11

Post by kyleburton »

jinushaun wrote: post #3

The code in question: for ( int i=0 ; i Nice analysis into the thinking that went into creating such bad code. Took me a while to even see the i-- at the bottom.

One of the advantages I find in pair-programming is that the 'navigator' (the one who's not typing) will often catch this kind of thing as it is happening. "Hey, that's just a while loop, or better yet just use `Clear`". There are other (greater) advantages to pairing, but this example is something we typically avoid before it gets committed.

Re: What's wrong with this code, really?

#12

Post by raganwald »

The article's point about writing code that does what it says it does is fine. But as an interview question, I have trouble imagining that candidates won't figure out that this is a game of "guess the answer I'm looking for" and say that they would rewrite this code.

A better question would be, there's tremendous deadline pressure, the company is in imminent danger of losing a giant deal if we don't have working code for some demo, and you have three features to implement by Monday afternoon. Do you write a new feature immediately, open a ticket for refactoring this loop and then write a new feature, or rehearse your explanation to the big boss that over the lifetime of the software, rewriting the code before adding a new feature was more important?

http://raganwald.posterous.com/javas-comb-over

Just kidding, but trying to make the point that "what do you think of this code" is a little obvious as an interview question.

Re: What's wrong with this code, really?

#13

Post by palish »

yardie wrote: post #4

Anyone else read it and on the first pass think, "yeah that works". Then on second pass think, "it's not a good idea, but it works." Then finally think, "under pressure I've done worse; at least this works as intended. S/he should probably comment it." Or is it only me? Addendum: I would also add that even as a junior program, Clear() was easily learned within the first few minutes and usually when you have to use a…

No, I pretty much just recoiled in horror immediately.

Even if I had to avoid .Clear(), there are immediately-obvious, better ways.

  while ( !thing.Empty() )
    thing.Remove( 0 );
In fact, the code isn't just bad; it's risky. What if someone changes "int" to "uint"? It wouldn't even infinite loop / crash... It would remove exactly one element!

Re: What's wrong with this code, really?

#14

Post by KiwiCoder »

raganwald wrote: post #12

The article's point about writing code that does what it says it does is fine. But as an interview question, I have trouble imagining that candidates won't figure out that this is a game of "guess the answer I'm looking for" and say that they would rewrite this code. A better question would be, there's tremendous deadline pressure, the company is in imminent danger of losing a giant deal if we don't have working code…

Most do say exactly that, then we have a chat about what they might do instead. Many immediately say, "um, is there a Clear or RemoveAll method?!" It's all over within a few minutes, we move on. The bigger picture is always going to trump the details, until the day the details have piled up and can no longer be ignored. It's the great big technical-debt elephant in the room.

Re: What's wrong with this code, really?

#15

Post by raganwald »

KiwiCoder wrote: post #14
raganwald wrote: post #12

The article's point about writing code that does what it says it does is fine. But as an interview question, I have trouble imagining that candidates won't figure out that this is a game of "guess the answer I'm looking for" and say that they would rewrite this code. A better question would be, there's tremendous deadline pressure, the company is in imminent danger of losing a giant deal if we don't have working code…

Most do say exactly that, then we have a chat about what they might do instead. Many immediately say, "um, is there a Clear or RemoveAll method?!" It's all over within a few minutes, we move on. The bigger picture is always going to trump the details, until the day the details have piled up and can no longer be ignored. It's the great big technical-debt elephant in the room.

I expected that would be the case, just wanted to make it obvious. Meta point: Context matters for code as well as interview questions :-)

Re: What's wrong with this code, really?

#16

Post by Lagged2Death »

yardie wrote: post #4

Anyone else read it and on the first pass think, "yeah that works". Then on second pass think, "it's not a good idea, but it works." Then finally think, "under pressure I've done worse; at least this works as intended. S/he should probably comment it." Or is it only me? Addendum: I would also add that even as a junior program, Clear() was easily learned within the first few minutes and usually when you have to use a…

If I'd had to resort to iteration for this, I would have included a prominent, bitchy comment/complaint pointing out how stupid it was that there was no Clear() call or that the Clear() call wouldn't work in this case, etc.

Modifying a loop variable inside a for() loop is generally a very bad idea, that's a red flag for me.

Re: What's wrong with this code, really?

#17

Post by hasslblad »

As soon as I saw that snippet I could see what's wrong.

In C# / .Net you can't remove an element from an enumerator while you're enumerating through it. You can remove the last element however, as it's the final loop the enumerator isn't used again so it won't throw an error. The original developer probably tried to remove it forward only first, encountered an error and wrote the code to loop through it backwards, using the random tweaking technique.

What's rather depressing is that a lot of developers I've encountered use the random tweaking methodology, instead of figuring out what's really happening.

Re: What's wrong with this code, really?

#19

Post by arethuza »

"Let’s pretend for a moment that we are a harassed contract programmer working late, under intense pressure to deliver working code before we can go home."

I would hope that in those kinds of situations I would remember to add a FIXME comment so that I would come back in saner times and make it nice.

Post reply on HN