What's wrong with this code, really?
cvmountain.com
What's wrong with this code, really?
1–10 of 217 posts
Re: What's wrong with this code, really?
#2Like 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.
Re: What's wrong with this code, really?
#3The code in question:
for ( int i=0 ; i Re: What's wrong with this code, really?
#4Anyone 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.
Re: What's wrong with this code, really?
#5Dependent 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.
Re: What's wrong with this code, really?
#6Like 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…
Another option would be to run a while() loop that keeps running till the current .count() value is == 0.
Re: What's wrong with this code, really?
#7Deleting from a container while you are iterating over it should always raise red flags.
Re: What's wrong with this code, really?
#8nice 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
Re: What's wrong with this code, really?
#9Like 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…
The 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...
Re: What's wrong with this code, really?
#10Like 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 and working for our jobs. I was happy to see the author include note of that rather than just call the original programmer an idiot for not knowing how to write maintainable code.