¶"Fixing" working code
One of the lessons you eventually learn the hard way as a programmer is never to "fix" working code. If you have a confirmed bug, then fix it; if you have a feature you need to add, then by all means, change it.
"Fixing" code that just looks buggy, without actually knowing how it is bad, is a mistake.
Let's say you have an 80% chance of getting code right in the first place, and thus a 20% chance of getting it wrong. That means if you just look at it again, you have a 64% chance of verifying it right again, a 16% chance of writing it incorrectly but catching the bug, and a 4% chance of blowing it both times. That leaves a 16% chance of writing it correctly and thinking it's wrong. There's then a chance that you'll act on that instinct and break working code.
Here's a real example that I just discovered:
A certain vendor's C runtime library has code to display an error dialog under certain circumstances, such as an assert, with a program path. If the program path is too long, it truncates it and only displays the end of the path with an ellipsis at the start. This was done via strncpy():
if (strlen(path) > MAXLENGTH) {
path += strlen(path) - MAXLENGTH;
strncpy(path, "...", 3);
}
In a later version of the product, someone noticed the use of strncpy() and replaced it with strncpy_s():
if (strlen(path) > MAXLENGTH) {
path += strlen(path) - MAXLENGTH;
strncpy_s(path, "...", 3);
}
Now, I'm not really a fan of the secure string library -- I'd rather just use a string class instead -- but strncpy() is admittedly a bit risky to use given its tendency to create non-null terminated strings. You can use strncpy() safely, but it's easy to forget or goof the boundary check. Safer alternatives like strlcpy() are often used instead. At first glance, the change looks like a perfectly good improvement to eliminate any potential buffer overrun mistakes... except for two problems. One, the source is a string literal and the destination is a fixed buffer that is far larger than four characters, so there was never any issue with non-termination. Second, the original code actually depended on strncpy()'s non-terminating behavior, because it is putting the ellipsis at the beginning of the string. The attempt to fix a non-existent bug actually broke the code.
So, basically, don't guess at bugs. If you think there is a bug, reproduce it first so you can actually confirm it and verify that you actually fixed something instead of breaking it, and then keep that case around for regression testing.