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.
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…
1. No iterator is being used here, so the iterator coherence check does not come into play
2. List elements are removed from the front, the code is essentially a complicated version of:
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…
Actually, that's not the problem :) The can't-modify-a-collection-while-enumerating-it issue only comes into play if you're actually using an enumerator (either directly, or as part of a foreach loop) - the code in the article uses a plain for loop along with indexing into the collection, and wouldn't run into the problem. Rather, the primary "issue" is that without that "i--" at the end it only removes half the elem…
Sorry, I should have been clearer. I was getting flashbacks to when I saw a similar problem (except in a for each loop), that's what set the alarm bells ringing in my head.
When I see nasty code like that, I tend to stop parsing it fully and sniff out the intent. I think it's a form of bad code blindness (like banner ad blindness) my brain is protecting me from all the bad code I've seen. If I fully parsed all the really bad code properly I’d become a dribbling wreck. :) So I tend to look at it at a higher level instead to stay sane.
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…
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.
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.
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.
I think that just touches on documentation, extensive commenting, and generally writing straightforward code. I didn't really know the importance of this until I had to step-by-step rewrite my thesis after it fell apart completely. Now, I make sure to document at the least a small explanation of what my lines of code mean and the end result, no matter how simple.
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.
But we are talking about tabs here. Premature optimization is the root of all evil (to throw another cliché).
Won't this.MyControl.TabPages[i] be invalid once i becomes greater than what this.MyControl.TabPages.Count now is? Since the "always remove element 0" code from the article works, I expect your code won't work because when you remove an element form the list, all subsequent elements now have an index one less than before, so after j/2 elements, you'll overrun the list.
Correct. I think the logical thing to do would be to delete from the end so that the index is always valid. for (int i=this.MyControl.TabPages.Count - 1; i > 0; i--) { this.MyControl.TabPages.Remove(this.MyControl.TabPages[i]); } Imagine the count is 2. The first iteration you delete item 1, the second you delete item 0, and then the loop exits. EDIT: Actually, as someone else pointed out, it's clearer to use a while…
Actually, you will never delete the final element with that code.
I think what you meant was:
for (int i=this.MyControl.TabPages.Count; i > 0; i--) { this.MyControl.TabPages.Remove(this.MyControl.TabPages[i-1]); }
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.
Yep, that's correct. I wish "thing.Length - 1" were as clear as "0", but it's not.
Besides, if you really cared about efficiency, you wouldn't be using growable containers... =)
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.