Hacker News Reader

Comment by yardie | Hacker News Reader

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.

View on HN

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é).

Spoken by a man who has never encountered a "tabbed notebook"-style UI with multiple rows of tabs, stacked on top of each other. ;)

Premature 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.

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.

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.

> 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).

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... =)

There'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).

This 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.

He post you are actually responding to answers your questions. Javascript and Python are mentioned.

"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.

What 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] );
  }

Oh, yep. That's correct, thanks.

If 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.

I 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.

The 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.

Perhaps 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.

> 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.

If 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

"readability < does it work"

No. In a professional setting, readability ~= does it work.

The code presented works almost by accident. It is clear there is no thought put into it at all. It is confusing, and subsequent modifications could cause errors very easily. It is not acceptable code.

Of course, code needs to work. Of course, if this .Clear method is a memory leak you wouldn't use it. Of course, we need to consider these things. But there is no situation in which this code is acceptable. This code would not even warrant a passing grade in an "Introduction To Programming" class.

> Of course, if this .Clear method is a memory leak you wouldn't use it.

Yes. Exactly.

> using invariants is only possible if you have immutable data structures.

Wow, dude, maybe if you don't know what the words mean, you shouldn't argue about them.

What business do you have using a vector of bare pointers anyway? If you use smart pointers, the container's clear() will automatically free resources held by container elements.

Desire to avoid garbage collection and automatic memory management mostly.

Reply on news.ycombinator.com