5 ms·
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
by wccrawford 15y ago
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.
- gallamine 15y agoAnother option would be to run a while() loop that keeps running till the current .count() value is == 0.
- shabble 15y agoThe 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_Painter%27s_algorithm https://secure.wikimedia.org/wikipedia/en/wiki/Schlemiel_the...
- jbri 15y agoThough 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...
- kzrdude 15y agoYou're removing i+1 elements here :)
- willfarrell 15y agofor(i = list.size; i--;) list.removeAt(i);
- Confusion 15y agoThat really depends on the implementation of the linked list. I don't think it would actually behave O(n^2) in most implementations.
- masklinn 15y ago> 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)
- icebraining 15y ago> 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.html http://www.docjar.com/html/api/java/util/LinkedList.java.htm...
- jeffesp 15y agoBesides 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.
- wccrawford 15y agoI'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.
- cunac 15y agoseems more like continuous rush :-(
- drv 15y agoThis 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...