Hacker News Reader

Comment by wccrawford | Hacker News Reader

Like the blog author, I thought it was obvious what the problem was: Every time I read that, I'm going to have to figure out what it means. Any time there's a problem or change to code in that area, I have to stop and understand what it's doing.

To clean it up, I'd do 1 of 2 things: Either write a .clear() function, or rewrite it to start at the end and clear the items in reverse.

With the .clear() function, I can at least ignore it because it was tested and worked. (You do write tests, right?)

With it in reverse, it's something I've done numerous times because of how lists work in certain languages. I'd instantly recognize that it's going backwards because the list always starts at 0.

If I wrote it in reverse, I'd also write a comment about why it's in reverse, though, so that anyone else can instantly know why, as well.

View on HN

The other nice thing about running it in reverse is that the .count method (assuming it's a method and not a property), needs only to be accessed in the initial condition setup.

If you're accessing the size of, say, a linked list, you end up sneaking an O(n^2) runtime because it has to re-count the size of the list every iteration to check for termination.

c.f. https://secure.wikimedia.org/wikipedia/en/wiki/Schlemiel_the...

Though you have to be careful with that, because the obvious code for removing from a collection in reverse:

  for(i = list.size; i >= 0; i--)
    list.removeAt(i);

Is itself a schlemiel algorithm on any singly-linked list...

You're removing i+1 elements here :)

for(i = list.size; i--;) list.removeAt(i);

That really depends on the implementation of the linked list. I don't think it would actually behave O(n^2) in most implementations.

> If you're accessing the size of, say, a linked list, you end up sneaking an O(n^2) runtime because it has to re-count the size of the list every iteration to check for termination.

You're still going to run in O(n^2) if you remove from the back (as you'd do if you run in reverse).

Linked list are far more efficient if you build in reverse but remove in iteration order (just replace your head pointer with head.next)

> If you're accessing the size of, say, a linked list, you end up sneaking an O(n^2) runtime because it has to re-count the size of the list every iteration to check for termination.

I'd expect any decent linked list implementation to keep an integer with the current size. Java certainly does: http://www.docjar.com/html/api/java/util/LinkedList.java.htm...

Another option would be to run a while() loop that keeps running till the current .count() value is == 0.

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 and working for our jobs. I was happy to see the author include note of that rather than just call the original programmer an idiot for not knowing how to write maintainable code.

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 :-(

This is broken in a different way (assuming you mean something like the following):

  int c = this.MyControl.TabPages.Count;
  for ( int i=0; i < c; i++ )
  {
     this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] );
  }

Let's say you have 4 items. That'll go something like this:

  Original list: (0,1,2,3)
  Remove 0    Item 0 removed, items (1,2,3) become (0,1,2)
  Remove 1    Item 1 (originally 2) removed, items (0,2) become (0,1)
  Remove 2    Only two elements left in the list (0 and 1), so there is no index 2 anymore...

Reply on news.ycombinator.com