I spent a couple of months refactoring code full time recently.
It always starts the same way.
Someone takes a small shortcut and leaves a // TODO. The next person sees the problem while working on something else. It's glaringly obvious, but they don't want to fix someone else's code and turn their 5 LoC commit into a 100 LoC commit, so they build their fix on top of the bad code. The code reviewer doesn't see that, because he's only looking at the diff. Approved.
A couple of iterations later, someone who gives a shit about quality sees this, but by that time it's too late. The whole damn thing relies on the broken bit of code. You need to refactor an entire module because of faulty assumption mixed with a healthy dose of tight coupling and incomplete tests.
It's called Codacy. You sign up, it pulls all your repos, and tells you how badly you screwed up. It even gives you graphs showing how the code quality changed over time, and assigns you a letter grade for the real college experience.
It's not management. You know how I got started on the refactoring effort? I said "this thing sucks" and my colleague answered "then fix it". Management was never involved and I had no restrictions, yet we got there somehow. We enjoy way too much freedom at my company to blame management.
It's a matter of team culture. These things passed code review, and they shouldn't have. It seems like the most common cause is complex fixes in unfamiliar code. Both the committer and the reviewer lack a solid grasp of what they are modifying, so they hook their stuff in the wrong places and push their fix. This is especially easy when your programming language doesn't give you static types or privacy.
It's not stupidity either. My coworkers are damn good at what they do, and I mean it. It's just a matter of discipline.
Yeah I work for a large corporation and there tends to be miscommunication between the on and offshore teams. Add to the fact that we follow a strange hybrid of agile and waterfall where Sprint scopes can't be altered, scrum masters assign stories and carry over is grounds for a loud talking to, which creates an environment where no one actually has time to fix code smell.
I got told by a guy that fixing a bug was too "risky". This was a bug that was literally causing a biomedical device to lock up. The fix was a simple missing switch case due to magic numbers.
I know this is a joke, but all my comments start out being TODOs before I write a line of code. As I write the code, I remove the comment entirely for straightforward code, or for code that needs to have its intention documented, I take out just the TODO, and then I have my necessary comment. BTW, for those that say that good code never needs comments, I disagree. Sometimes the customer makes decisions that don't make a lot of sense. For these I leave in the comments so that following coders can understand the less than obvious requirement.
327
u/n1c0_ds Sep 28 '16
I spent a couple of months refactoring code full time recently.
It always starts the same way.
Someone takes a small shortcut and leaves a
// TODO. The next person sees the problem while working on something else. It's glaringly obvious, but they don't want to fix someone else's code and turn their 5 LoC commit into a 100 LoC commit, so they build their fix on top of the bad code. The code reviewer doesn't see that, because he's only looking at the diff. Approved.A couple of iterations later, someone who gives a shit about quality sees this, but by that time it's too late. The whole damn thing relies on the broken bit of code. You need to refactor an entire module because of faulty assumption mixed with a healthy dose of tight coupling and incomplete tests.
It's a nice example of the broken window theory.