6 ms·
Anyone 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
by yardie 15y ago
Anyone 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 agoSpoken by a man who has never encountered a "tabbed notebook"-style UI with multiple rows of tabs, stacked on top of each other. ;)
- lukeschlather 15y agoPremature optimization is the root of all evil, but removing from the beginning of any array-based growable structure rather than the beginning will turn a O(n) operation into an O(n^2) operation. And you don't know what's going into those tabs. That cliché shouldn't be used to excuse positively brain-dead choices.
- danieldk 15y agobut removing from the beginning of any array-based growable structure rather than the beginning will turn a O(n) operation into an O(n^2) operation. I'll bite: in arrays, front deletion (like back deletion) can be done in O(1) time. Keep a pointer to the first element, and just increment it upon front deletion (and free the element if necessary). Obviously, you may want to be smart to avoid memory use getting out of hand. And you don't know what's going into those tabs. If the array stores references (pointers), which you'd expect with tabs, that is orthogonal to the discussion. Whatever is going into those tabs, a pointer is a pointer.
- xentronium 15y ago> I'll bite: in arrays, front deletion (like back deletion) can be done in O(1) time. Keep a pointer to the first element, and just increment it upon front deletion (and free the element if necessary). Obviously, you may want to be smart to avoid memory use getting out of hand. Actually that's how they often do array-based queues and deques. To help with memory use they let the tail pointer overflow through the end (so you could have tail at index 2 and head at index 8).
- deleted 15y ago[deleted]
- palish 15y agoYep, 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... =)
- bad_user 15y agoThere's usually a Remove() with no arguments that depending on the data-structure involved will remove from either the beginning or the end (depending on how it is more efficient for said data structure).
- roel_v 15y agoThis 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.
- lurker19 15y agoHe post you are actually responding to answers your questions. Javascript and Python are mentioned.
- burgerbrain 15y ago"Lists" in python are not in fact lists (as the term is traditionally used to refer to linked lists). They are array backed. I expect "arrays" in javascript are similarly arrays... roel_v is correct to point out this misunderstanding.
- tspiteri 15y agoWhat if someone changes "int" to "uint"? It wouldn't even infinite loop / crash... It would remove exactly one element! I can't see that. If i is a uint, at the end of the first cycle, i-- changes i to the maximum uint, then i++ changes i to 0, then i < Count compares Count to 0. It is identical to, if uglier than, the following: while ( 0 < this.MyControl.TabPages.Count ) { this.MyControl.TabPages.Remove ( this.MyControl.TabPages[0] ); }
- palish 15y agoOh, yep. That's correct, thanks.
- Lagged2Death 15y agoIf I'd had to resort to iteration for this, I would have included a prominent, bitchy comment/complaint pointing out how stupid it was that there was no Clear() call or that the Clear() call wouldn't work in this case, etc. Modifying a loop variable inside a for() loop is generally a very bad idea, that's a red flag for me.
- yardie 15y agoI agree I think this implementation is completely sloppy. At the same time I guess I've been desensitized to the word "wrong". To me, wrong just means the results are wrong or it doesn't work. In this instance it actually does work, but there are dozens of other ways that would have worked without giving you the Coding Horror face.
- KirinDave 15y agoThe code was unacceptable from line 1 onwards, and would not pass code review with me on watch. For one, for loops have invariants. Next up, you are altering the loop bound in the loop. I do not care if the code works, the code is hard to reason about when it goes all non-linear like that. Finally, there is a goddamn Clear method. That is the kind of code you see the next morning and delete; hoping none of your peers review the version control log that closely.
- trevelyan 15y agoPerhaps the programmer wanted to run test functions on the objects as he deleted them? Or perhaps the data structure is a vector of pointers in C++, in which case using the standard clear method would introduce a massive memory leak. There are a lot of reasons why someone doing difficult work with complex objects would use a loop to delete them, and using invariants is only possible if you have immutable data structures.
- KirinDave 15y ago> Perhaps the programmer wanted to run test functions on the objects as he deleted them? Doesn't justify anything that was done. > Or perhaps the data structure is a vector of pointers in C++, in which case using the standard clear method would introduce a massive memory leak. Doesn't justify anything that was done. > There are a lot of reasons why someone doing difficult work with complex objects would use a loop to delete them Then use a while loop, and make this explicit. While loops have tests, for loops have invariants. There is a reason both exist, and it's not purely historical. > and using invariants is only possible if you have immutable data structures. Incorrect.
- trevelyan 15y agoIf someone used clear on a vector of pointers AS RECOMMENDED IN THIS ARTICLE they've just introduced a serious, serious problem. readability < does it work
- 15y ago