13 ms·
What's wrong with this code, really?
- pointyhat 15y agoThis is perfectly valid code. You cannot modify a collection which is being iterated safely so it's the best way to handle the situation.
- deleted 15y ago[deleted]
- mekoka 15y agoThe 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 take another 2 minutes. The point of the article isn't really the method that was used to get to the result, but rather the fact that code should be made easy to read and understand, because +60% of the time is spent maintaining it. I usually tell this to newbie programmers, "code is meant for people to read, machines understand on/off". Even if you need to borrow such a convoluted approach to clear a collection (as opposed to the more direct clear() method), there are simpler and more readable alternatives: while(Pages.count > 0){ Pages.Remove(Pages[0]); }
- pointyhat 15y agoThat's what comments are for. If something is ambiguous, then you should comment it. As for your approach, I do like that better.
- MortenK 15y agoAbout 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 until they work", as lesser, more inefficient or just plain bad programmers. Just remember that you were once like that too. I think there should be more patience among the experienced, for the programmers who are still learning the basics.
- jcromartie 15y agoI think there's a fundamental concept that isn't taught very well, and that concept is: Computers only ever do (for something like 99.999999999% of instructions) exactly what they are told to do. They don't have a mind of their own, and programming isn't magic. Opaque languages, libraries and APIs don't help the situation either. I wonder how many programmers start out under the assumption that computers are more-or-less magic?
- outworlder 15y agoI've noticed this "magic" thing too, among the most clueless members of our profession. Even if they have no idea how bad they are, which they usually don't.
- sanderjd 15y agoYou see this a lot (even on this forum) with people referring to Rails "magic", when it can't possibly be anything but a bunch of lines of code being executed according to the rules of a certain interpreter.
- pjscott 15y agoTo paraphrase Agatha Heterodyne, any insufficiently analyzed technology is indistinguishable from magic. I think their terminology is pretty reasonable.
- sanderjd 15y agoIsn't this identical to what the original post I replied to was saying - those he refers to as the "clueless" have not sufficiently analyzed enough technology to distinguish it from magic. I wouldn't go so far as calling people clueless though, thinking of some things as magical black boxes frees up mental space.
- mseebach 15y agoRails magic is vastly different from the magic referred to in the gp. Rails uses a number of dynamic approaches that create results that in other frameworks would take some amount of explicit configuration. The word "magic" in this context is ironic and merely means "dynamic".
- altrego99 15y agoHe got underpaid and bad boss right, but more likely this could be due to frustation. I have seen a coder who uses many different ways to code simple things, for example in a code he used (a and b), (a+b>=2), (1-a*b), and several other ways to do the same thing.
- ibisum 15y agoSequence points, kids. Learn to recognize them.
- wccrawford 15y agoLike 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...
- jinushaun 15y agoThe code in question: for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ ) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] ); i--; } Nice analysis into the thinking that went into creating such bad code. Took me a while to even see the i-- at the bottom.
- kyleburton 15y agoOne of the advantages I find in pair-programming is that the 'navigator' (the one who's not typing) will often catch this kind of thing as it is happening. "Hey, that's just a while loop, or better yet just use `Clear`". There are other (greater) advantages to pairing, but this example is something we typically avoid before it gets committed.
- yardie 15y agoAnyone 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 hack like this it's because something has gone wrong. I wouldn't necessarily chalk this up to inexperience or deadline it could honestly be there was a bug and this was the only way to get it to work.
- palish 15y agoNo, 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!
- simonsarris 15y agoJust 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.
- deleted 15y ago[deleted]
- deleted 15y ago[deleted]
- danieldk 15y agoBut we are talking about tabs here. Premature optimization is the root of all evil (to throw another cliché).
- lylejohnson 15y ago
- hendrik-xdest 15y agoDependent on the state of mind, one might even have done something like this (as I can't tell the language used in the example): this.MyControl.TabPages = new Array(); Try to find something like this when the problem you are confronted with is that a server has to be rebooted every few hours because it eats up memory.
- praptak 15y agoDeleting from a container while you are iterating over it should always raise red flags.
- morsch 15y agoDepends 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.
- JoachimSchipper 15y agoEven 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);
- hxa7241 15y agofor (i = ctr_size(container); i--; ) if (!predicate(container, i)) ctr_remove(container, i);
- JoachimSchipper 15y agoThat's arguably better. Nitpick: start at ctr_size(container) - 1. [Feel free to edit your post, and I'll just delete this one.]
- BrandonM 15y agoNope. The decrement is occurring in the test, so it occurs after the initialization and before the first iteration.
- sid0 15y agofilter predicate xs :)
- ChrisArchitect 15y agonice writeup, could really feel your pain/obsession (not a bad thing) -- the sketchiness of the codeblock from the get go was cringeworthy for me too - harks to marking CS assignments and the like back in the day. what a way to start my day too. blech
- raganwald 15y agoThe 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 for some demo, and you have three features to implement by Monday afternoon. Do you write a new feature immediately, open a ticket for refactoring this loop and then write a new feature, or rehearse your explanation to the big boss that over the lifetime of the software, rewriting the code before adding a new feature was more important? http://raganwald.posterous.com/javas-comb-over http://raganwald.posterous.com/javas-comb-over Just kidding, but trying to make the point that "what do you think of this code" is a little obvious as an interview question.
- ajross 15y agoSurely the right answer would be something to the effect of "If this code survived in the source base until deployment, all is lost anyway." The point of code review is to catch monsters like this when they are written. If that doesn't happen, what's the point?
- Tyrannosaurs 15y agoThat's a very purist view of the world. This code works and, as he says, was produced by a coder under time pressure. You show me a system and pretty much I'll show you a system that has poor (but working) code in it produced by a competent developer under pressure. And code review is a useful process but it's no guarantee that issues will be caught any more than system testing or user acceptance testing. And yet the world keeps on spinning and all is not lost. I agree, you'd hope this was caught, but if someone gave that answer in an interview even aside from the tone, I'd wonder how much real world experience they had.
- KiwiCoder 15y agoMost do say exactly that, then we have a chat about what they might do instead. Many immediately say, "um, is there a Clear or RemoveAll method?!" It's all over within a few minutes, we move on. The bigger picture is always going to trump the details, until the day the details have piled up and can no longer be ignored. It's the great big technical-debt elephant in the room.
- hasslblad 15y agoAs 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, using the random tweaking technique. What's rather depressing is that a lot of developers I've encountered use the random tweaking methodology, instead of figuring out what's really happening.
- jbri 15y agoActually, 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 elements - after removing an element, all the following elements shift back one index, and so the very next element never gets removed.
- hasslblad 15y agoSorry, 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.
- masklinn 15y ago1. 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: while (0 < this.MyControl.TabPages.Count) { this.MyControl.TabPages.Remove(0); }
- qntm 15y agoWhat's wrong with the code, then, is that it was written under a little too much pressure for the developer to think clearly. for ( int i=0 ; i < this.MyControl.TabPages.Count ; ) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] ); }
- AlexandrB 15y agoWhy not just: while (this.MyControl.TabPages.Count > 0) { this.MyControl.TabPages.Remove ( 0 ); }
- masklinn 15y agoVery inefficient on most array lists (but a very good idea on linked lists)
- tomjen3 15y agoHow many tabs do you use? Less than 50? Then don't worry about the optimization until you have to port it to a PDP10.
- tlrobinson 15y agowhile (this.MyControl.TabPages.Count > 0) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages.Count-1 ); }
- hasslblad 15y agoOr Even this.MyControl.TabPages.Clear();
- arethuza 15y ago"Let’s pretend for a moment that we are a harassed contract programmer working late, under intense pressure to deliver working code before we can go home." I would hope that in those kinds of situations I would remember to add a FIXME comment so that I would come back in saner times and make it nice.
- suivix 15y agoWow, I would never make a for loop like that. It is weird and breaks convention, and has a high chance of causing a bug.
- onemoreact 15y agoThat 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.
- dean 15y agoThat'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.
- droz 15y agoMost projects are abandoned due to political reasons. Very rarely do they get dismissed because someone didn't code to whatever standard of the moment.
- onemoreact 15y agoExactly if you spend 2 years and 10 million developing some new HR software that IBM spends 5million / year supporting for the next 18 years then that's a success even if you spent 90% in support in fact the better the project is the more likely for you to spend more money supporting it after the initial release. The only way to reduce that cost is to spend so long designing your software that it’s never actually released.
- aptwebapps 15y agoSo it's a win-win?
- mrspeaker 15y agoI've seen this construct used back-in-the-day in C, but usually with conditional expressions before the remove. Perhaps it was just a crap bit of code from day one, but it's possible that devolved to that state over time.
- throwawayday 15y agowow - my first thought was something unprintable. Took a few minutes of staring at it before I could figure out what the code was doing. brlewis has the winning answer
- HarrietJones 15y agoInteresting that you say what's wrong with the code, but don't actually say how it should be done right.
- KiwiCoder 15y agoJust in case you're talking to me, HarrietJones, I do actually say how it could be done: with the Clear() method.
- hackinthebochs 15y agoI think this is more of an indictment of how we code rather than the programmer. There is nothing fundamentally wrong with code-by-experimentation. With libraries and frameworks growing in complexity over the last decade or so, it's all but impossible to hold all the details in your head of whatever piece of abstraction you're computing with. With dynamic languages and REPLs, it becomes even more standard to experiment until we get the correct result. The problem is that imperative programming is horrible for code-by-experimentation. You end up with code that works, but is hideously unreadable. Declarative styles can help greatly with this. Functional programming can be a big boon here. But I think we're going to need a fundamental shift soon in either tool quality (say, to automatically refactor that shit code into the most straightforward and readable way), or a new paradigm that will allow code-by-experimentation to always result in readable code.
- juaninfinitelop 15y agoMy initial thought was... If it has a .Count() and a .Remove(), it should have a .Clear()
- napierzaza 15y agoAllocating an int?
- tlrobinson 15y agoSo what's the "correct" way to do this? Clearing the entire array can usually be accomplished easily, but what if you want to remove only items matching some condition? Looping backwards, perhaps? for ( int i=this.MyControl.TabPages.Count-1 ; i >= 0 ; i-- ) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] ); }
- sid0 15y agoAs I mentioned above, the best way to keep items matching a condition is filter predicate xs
- IvoDankolov 15y agoIn case you don't speak Haskell: IEnumerable<T>.Where(Func<T, bool> predicate) It's quite simple to use: var odd_numbers = numbers.Where( n => n%2 == 1 );
- keltex 15y agoI find this a lot with HTML guys I work with. They add a few px of padding to the top of something to get it vertically centered on Chrome and then it's broken on IE. Then I tell them to fix IE and then it's broken on Chrome. Then it's two more hours of screwing around until they get it right. Then I show it to them on a notebook with a different DPI setting...
- chids 15y agoRelated to the part about software maintenance in the last part of the post I recently wrote about "visualizing cost and improvement areas for software maintenance" here: http://marten.gustafson.pp.se/content/visualizing-cost-and-improvement-areas-for-software-maintenance http://marten.gustafson.pp.se/content/visualizing-cost-and-i...
- dcosson 15y agoGreat post - I can get pretty OCD about the way code is written, but I have a hard time complaining about things like this without feeling like a dick since as you pointed out it's not particularly inefficient (even if there was an O(1) Clear() method, how many TabPages are we really working with that it would matter?) But you've reassured me that it's a reasonable thing to do, especially if I know beforehand that it's a piece of code that will probably be used for a long time. That said, I've learned that with an early stage startup where you're trying to iterate as quickly as possible in a desperate attempt to get somebody to care about your product, you often have to pick your battles. Just yesterday I came across this: category_count = [] for i in range(10): category_count.append( db.execute("SELECT count(*) FROM table WHERE category = %d" % i) ) For one thing, this iterate separately and then append to list approach in Python annoys me slightly (list comprehensions are so much cooler!). But far worse, it hits the DB 10 times instead of once, and no matter how small your site is you obviously can't be having that. How'd it get there? Who knows. It was written in the Django ORM, where the only way to do this is with a pretty obscure command like Object.values('category').annotate(count=Count('category')). At first we were picking up Django as we went, so at the time whoever wrote it probably had no idea that the values() or annotate() methods even existed, and the way it was written got something up on the page and working so we could decide whether or not we'd be throwing it out the next week. But, whatever, you come across something like this, go throw up, fix it and move on. And finding these kinds of issues puts into perspective smaller ones like using a for loop where you meant to use a while loop. tl;dr - Having the luxury of sexy-ing up your your code as described in the post is strongly dependent on the stage that the project/company is in.
- tcarnell 15y agoIf the code compiles and does what it is supposed to do, then the answer is that nothing is 'wrong' with it. Writing code that does what it is supposed to do is often not the challenge of software engineering - but writing code that can be easily tested, refactored, altered and ultimately understood by other developers is the harder part. The conditional statement used in the 'for' loop whose value can not easily be determined is not helpful and the i--; is 'unusual'. In any case, it is more useful to code review the unit tests than the code itself.
- g0su 15y agoIt's bad because you have to write a long blog post about it explaining all the pitfalls, compare good to bad programers, talk about maintenance cost, etc etc. This code: for i in 1.100: print i There's nothing to talk about, it's crystal clear.
- polshaw 15y agoOK, fairly newbie coder here.. What would be wrong with just setting a variable to the value of 'this.MyControl.TabPages.Count' outside of the for loop and refering to this?? ie; var x = this.MyControl.TabPages.Count; for ( int i=0 ; i < x ; i++ ) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i] ); } as a quick fix, or if someone did not know while loops or clear function??
- gredman 15y agoSay we start with three tabs, so i goes 0, 1, 2. When i reaches 2, two tabs have already been removed, so there is only one tab left. So in the loop body we then do: this.MyControl.TabPages[2] // oh no! In general, modifying a collection is a bit of a code smell, and a lot of iterator implementations will actually throw exceptions if you try it.
- polshaw 15y agoOK, i didn't think through the code.. rather i meant to iterate downwards; var x = this.MyControl.TabPages.Count; for ( int i=x ; i >0 ; i-- ) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages[i-1] ); }
- IvoDankolov 15y agoWell, what happens when you try to remove the 10th page when there's only 1 left? As a rule of thumb, don't ever rely on indexation in a collection if you do random deletes. Usually you'll just blow up your app gracelessly. Sometimes, epic failure ensues. Through some feats of logic we might deduce that there's always a first element, though, until the collection is empty. So you might do this inside the loop: MyControl.TabPages.RemoveAt(0) Needless to say, calling Remove when you have the bloody index (on IList collections that is) is counter-productive. And I guess that code would be okay. I mean, if you head to phrase it : let x be the number of elements in the list, take out the head of the list that many times. However, is it really the fastest way to clear a list? No. If we could access the class internals, we could just replace the store with a new empty array. Voila, O(1) clear and the garbage collector takes out the trash for you. Generally, though, don't spend time worrying about implementation if you already have one available. When you've done optimizing all of your stuff (which is never the case), then you could go on to suggest changes to the standard library.
- rohit89 15y agoWhenever a loop index is modified inside a for loop, it should raise immediate red flags. Also, with intellisense in Visual Studio, it shouldn't take more than a few seconds to check if there is .Clear() or .RemoveAll() method. That said, I've been guilty of doing stupid things like this many times when I'm tired and just want the damn thing to work. Its amazing the kind of errors you make in situations like that.
- hackinthebochs 15y agoYup. A for loop's index should be completely controlled by the control clause. Modifying that control variable in loop should trigger air raid sirens. It wouldn't surprise me if static analyzers triggered an error on that.
- saraid216 15y agoI'm more worried that there are a host of comments, both on the blog and here, by people with enough time to think this through who clearly aren't doing so. At the very least, I would have hoped everyone had read through the article.
- codeslush 15y agoWhen pressed for a deadline, a demo, a functioning "something" - I do stuff that might not be the right way to do things because I need to get it to work. If the code ever has a chance of being witnessed by someone else, I always try to: /* Can't find a clear/remove method, don't have time to screw around with it now, might revisit later, might not. Sorry. */
- codeslush 15y agoI should add that I often make these comments even if my code doesn't have a chance of anyone else looking at it. I forget way too often why I did something and need my own reminders. For all we know, this guy may have actually had a good reason to do this (work-around, faster, ...) - a simple qualifying comment for doing something out of the ordinary would have explained it all.
- schiptsov 15y agoYou need not to be a genius to immediately notice that doing i-- inside a for loop is.. OK just not very smart. ^_^
- buff-a 15y agoWhen I was asked to justify flagging this code Good grief. You are in a sorry environment. What are you doing there? I will fire anyone that writes code like that and checks it in, and then fire anyone who objects to me flagging it in a code review.
- gersh 15y agoConsider this: for ( int i=0 ; i < this.MyControl.TabPages.Count ; i++ ) { try { this.MyControl.TabPages.Remove (this.MyControl.TabPages[i] ); i--; } catch(Exception e) { } } Can this be simplified? If this syntax were used elsewhere in the program, but we didn't want to catch the exception in this particular case, should we copy-paste this, and remove the try block? Might there be a situation where preserving the syntax makes the program clearer?
- whackberry 15y agothat example code is terrible, it deserved no analysis of any kind.
- horseracer999 15y agoI love these types of articles.
- keyboarder 15y agoWould love to see more articles like this.
- arcanebook 15y agoCool analysis.
- redrising1 15y agoOh god I hope I never wrote code like taht when I first started out.
- deathmatch 15y agoIt makes me recoil in horror.
- ghostshield11 15y agoExcellent writeup.
- tealtank 15y agoSeeing code like this almost makes me want to give up programming.
- cosmicman 15y agoMaybe there should be a competition to see who can write the worst piece of code. :)
- gildedsuit 15y agoVery well written analysis.
- whiteduck 15y agoAs a newbie coder I find this type of post very illuminating.