As a newbie coder I find this type of post very illuminating.
What's wrong with this code, really?
211–217 of 217 posts
Re: What's wrong with this code, really?
#212Earlier quoted context omitted.
The answer isn't "I know an API or assume something about it correctly". The answer is "Gawd that's fucking awful, I can't believe there's not a clear() method. I'd go look, before reinventing the wheel. And if lib was internal I'd suggest adding the clear() method instead of adding horrible loops all over the code . And if 3rd party lib I'd question the quality and if we really should be using that lib". In otherwor…
Yes, that would be a great answer, and ideally they get to the part you mentioned about adding it to an internal library, to which the next question is, Ok, you're adding it, how do you write it? My point is merely that it's still worthwhile to see an implementation, rather than moving on when you hear the word "Clear".
Any implementation coded up on the fly, from memory in 30 min or less is crap. I don't want to hire people willing to write crap, so I don task them to.
Re: What's wrong with this code, really?
#213Earlier quoted context omitted.
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 peer…
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 structu…
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.
Re: What's wrong with this code, really?
#214You need not to be a genius to immediately notice that doing i-- inside a for loop is.. OK just not very smart. ^_^
Changing a loop counter in a loop is always a bad idea? You must implement only very simple algorithms.
Re: What's wrong with this code, really?
#215You need not to be a genius to immediately notice that doing i-- inside a for loop is.. OK just not very smart. ^_^
Changing a loop counter in a loop is always a bad idea? You must implement only very simple algorithms.
There are more than one loop expression. Idiomatic usage of a for loop doesn't include changing counter variable in a loop's body. ^_^
Re: What's wrong with this code, really?
#216Earlier quoted context omitted.
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 structu…
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.
Re: What's wrong with this code, really?
#217Earlier quoted context omitted.
Depends 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.
The correct idiom is delete list head until list is empty. The iterator may become invalid if the collection it derived from changes. Iterator delete methods are crazy to begin with since iteration does not correlate with deletion. But anyway it is not clear where the iterator's cursor will point after you delete the current element. You could end up deleting every second element in the container.
My example wasn't clearing the list, it's a filter. And depending on what language and library you're talking about, it's very clear where the cursor will point to after a delete. There's nothing crazy about it with an even halfway sane collection framework.