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

61–70 of 217 posts

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

#61

Post by cunac »

wccrawford wrote: post #44
jeffesp wrote: post #10

Earlier quoted context omitted.

Besides a .Clear(), wouldn't the second most obvious be to have a count variable external to the loop that is initialized to the number of tabs before entering the loop? Then you still count up, and are able to reach the end of the list. Either way, we have just come up with 3 much clearer solutions in what I would guess is at most 5 minutes between us. I would guess we have the luxury of it not being 9pm at night an…

I'm sure we've all had our 'Why did I code that so badly???' moments. ;) I stopped getting mad at people for bad code quite a while back. It doesn't help either of us. Instead, I just fix it. The bonus to me is the little rush from improving something. Thank goodness for that little rush.

seems more like continuous rush :-(

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

#62

Post by morsch »

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.

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

#63

Post by simonsarris »

thaumaturgy wrote: post #57
simonsarris wrote: post #35

Earlier quoted context omitted.

For the record, the second way is typically better: It is usually more (sometimes much more) efficient to remove from the end of a list/array than to remove from the beginning.

I'm genuinely curious -- why? The last time I coded my own linked-list, it was doubly-linked, so deleting either the head or the tail was exactly the same.

Its heavily implementation dependent, but for many implementations of lists and arrays shift is slower than pop. Shift tends to require moving the entire array around, while pop does not. It's rarely the case with doubly-linked lists, or with perl arrays (they do something special, keep the starting offset recorded or something)

For you case deleting from either end ought to be fine, but you've made the other implicit tradeoff because merely accessing items in the middle of a linked list will be slow. In the case of something like a JavaScript array, removing from the front is 80% slower than removing from the end:

http://jsperf.com/popvsshift

Same deal with Python lists. From the Python spec:

http://docs.python.org/tutorial/datastructures.html#using-li...

It is also possible to use a list as a queue, where the first element added is the first element retrieved (“first-in, first-out”); however, lists are not efficient for this purpose. While appends and pops from the end of list are fast, doing inserts or pops from the beginning of a list is slow (because all of the other elements have to be shifted by one).

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

#64

Post by roel_v »

simonsarris wrote: post #37
palish wrote: post #13

Earlier quoted context omitted.

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!

Just for the record, your method is inefficient for a lot of implementations of lists/arrays, where it is often far faster to remove from the end than to remove from the beginning.

This is the second time you said that in this thread, now I can't restrain myself anymore - 'list' != 'array', and it all depends on how they're implemented; I see no reason to assert that for 'most implementations' removing from the end is faster. For a single linked list, it's faster to remove from the start. For a double linked list, it doesn't matter (well it depends, it could be slower). For a 'we call it a list but all containers are really hashes', it doesn't matter. For a regular C array, it depends on how you work with it.

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

#65

Post by vilya »

thaumaturgy wrote: post #57
simonsarris wrote: post #35

Earlier quoted context omitted.

For the record, the second way is typically better: It is usually more (sometimes much more) efficient to remove from the end of a list/array than to remove from the beginning.

I'm genuinely curious -- why? The last time I coded my own linked-list, it was doubly-linked, so deleting either the head or the tail was exactly the same.

For a list there's not likely to be a difference. For an array you have to shift all the elements after the one you're removing down by one place; if you're removing from the start of the list you have to adjust every item in the list, but if you're removing from end you don't have to adjust any items.

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

#66

Post by roel_v »

hasslblad wrote: post #17

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, us…

Yeah well except that that's not even related to what's happening here. Good thing though that you spotted it immediately and that you're smarter than all the developers you encounter ^_^

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

#69

Post by icebraining »

thaumaturgy wrote: post #57
simonsarris wrote: post #35

Earlier quoted context omitted.

For the record, the second way is typically better: It is usually more (sometimes much more) efficient to remove from the end of a list/array than to remove from the beginning.

I'm genuinely curious -- why? The last time I coded my own linked-list, it was doubly-linked, so deleting either the head or the tail was exactly the same.

And for singly linked lists, deleting the last would be O(n), while the first would be O(1) (just change the head to point to the next node).

Parent has a point for arrays/arraylists, though, you need to copy everything after the element.

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

#70

Post by throwawayday »

RyanMcGreal wrote: post #32
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…

Understood, but I would caution that even an inexperienced, inefficient programmer should still understand what their code does and why it works . The best way the more experienced can exercise patience is by giving inexperienced programmers enough time to figure it out rather than pressuring them to flip random switches frantically.

it's called experience for a reason - the coder has to try (and fail) until they learn enough to code good, efficient, clear code
Post reply on HN