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

131–140 of 217 posts

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

#131

Post by dkersten »

IgorPartola wrote: post #119
dkersten wrote: post #55

Earlier quoted context omitted.

I prefer: for (int i=this.MyControl.TabPages.Count - 1; i >= 0; i--) { this.MyControl.TabPages.Remove(this.MyControl.TabPages[i]); } Though a simple while loop is much easier to follow, even if its less efficient than removing the elements in reverse.

Exactly. For example in JavaScript doing a something.length would result in re-counting the number of elements, while doing the loop in this style would efficiently store the count in a variable that is fast to access and manipulate. I suppose if you want to be a real optimization junky you'd also use --i instead of i--.

I suppose if you want to be a real optimization junky you'd also use --i instead of i--.

Ten years ago, sure, but nowadays I trust the compiler to do this for me ;-)

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

#132

Post by sunir »

morsch wrote: post #62
praptak wrote: post #7

Deleting from a container while you are iterating over it should always raise red flags.

Depends on what you want to accomplish and what your container and iterators support. If your Iterator offers a delete method, I think it's a very elegant and clear way to filter a container. for (Iterator it = container.iterator(); it.hasNext(); ) if (!predicate(it.next())( it.remove(); It's more ugly and error prone if you've got to juggle an index, though.

The correct idiom is delete list head until list is empty.

The iterator may become invalid if the collection it derived from changes.

Iterator delete methods are crazy to begin with since iteration does not correlate with deletion. But anyway it is not clear where the iterator's cursor will point after you delete the current element. You could end up deleting every second element in the container.

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

#133

Post by seabee »

sanderjd wrote: post #125
simonsarris wrote: post #94

Earlier quoted context omitted.

Most of the time it is simply because a shift operation has to re-address each element in the array (if it is implemented like a ho-hum, classic array) and a pop operation does not have to do this.

I think the assumption was that even arrays indexed by integers are stored as JS Objects, which are basically a hashmap style structure, and so they wouldn't be implemented like a ho-hum, classic array. I'm not sure if that assumption is right, but if it is, there would be no need to re-address each element.

If you don't re-address each element, how do you map the value at index 1 to index 0?

One way is to leave the indexes intact and keep an offset around, so you may map the nth logical index to the nth physical one. (Add 1 on a shift, subtract 1 on an unshift.)

But if you wanted this behaviour, there are more efficient ways: http://en.wikipedia.org/wiki/Circular_buffer

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

#134

Post by pointyhat »

mekoka wrote: post #116
pointyhat wrote: post #97

This is perfectly valid code. You cannot modify a collection which is being iterated safely so it's the best way to handle the situation.

The code works, the problem is that it's not very obvious why it's going about it the way it does. You look at it and right away ask yourself "wtf, did I miss something?" simply because it's so unusual. You might look at it for 2 minutes and figure it out, but that's 2 minutes too long for what's actually being accomplished. The problem is, if you were to come back and look at it again 6 months from now, it would tak…

That's what comments are for. If something is ambiguous, then you should comment it.

As for your approach, I do like that better.

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

#135

Post by techdmn »

MortenK wrote: post #27

About the McConnell quote: "Inefficient programmers tend to experiment randomly until they find a combination that seems to work." The essence of this quote is being passed around quite often these days. When you first start programming, you generally have no idea what the hell you are doing. You learn all these strange, abstract concepts best, by experimenting. It's easy to dismiss people "jiggling things around unt…

Nothing wrong with having patience and helping new programmers, but my compassion ends there. :) I didn't know what I was doing when I started, but I learned it's worth the effort to understand _why_ something works once it does. Not to try is passing the buck, and I've spent plenty of time on the receiving end. I've seen "shotgun" code written by developers with 20 years of experience, and boy does it suck to maintain that:

"Wow, this code is messed up. This whole project is messed up. Wait, there's a weird edge case the author must not have considered. Hmmm, that has some side effects too. Crap, this can't be on purpose, but a bunch of other code touches this thing. Does any of it rely on this behavior?"

With any luck, after burning a day studying a thousand lines of code, you realize that none of it does anything useful, and the bug you were chasing is hiding somewhere else. (Ok, I'm a little better at debugging than that, but you see the point.)

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

#136

Post by aptwebapps »

dean wrote: post #82
onemoreact wrote: post #50

That chart of development costs ignores the fact that only successful projects get maintained. Many projects simply get abandoned before they ever gain traction and at that point code quality becomes meaningless.

That's a good point. And if the project consists of mainly bad code, it probably contributes to the project being abandoned, as it gets harder and harder to fix bugs and add new features in a code base like that.

So it's a win-win?

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

#137

Post by trevelyan »

KirinDave wrote: post #103
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…

The code was unacceptable from line 1 onwards, and would not pass code review with me on watch. For one, for loops have invariants . Next up, you are altering the loop bound in the loop. I do not care if the code works, the code is hard to reason about when it goes all non-linear like that. Finally, there is a goddamn Clear method. That is the kind of code you see the next morning and delete; hoping none of your peer…

Perhaps the programmer wanted to run test functions on the objects as he deleted them? Or perhaps the data structure is a vector of pointers in C++, in which case using the standard clear method would introduce a massive memory leak.

There are a lot of reasons why someone doing difficult work with complex objects would use a loop to delete them, and using invariants is only possible if you have immutable data structures.

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

#138

Post by spokengent »

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…

The correct answer is "This code looks ugly, and bad".

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

#139

Post by romac »

WildUtah wrote: post #121
JoachimSchipper wrote: post #88

Earlier quoted context omitted.

Even with plain C, you can just iterate backwards: for (i = ctr_size(container); i > 0; i--) if (!predicate(container, i - 1)) ctr_remove(container, i - 1);

Use the goes-to-zero operator. for (i = container.size; i-->0;) if(deletep(container, i)) container.remove(i);

Reminds me of something :)

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

#140

Post by pjscott »

spokengent wrote: post #138
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…

The correct answer is "This code looks ugly, and bad".

Thank you for making this as simple as it should be.
Post reply on HN