4 ms·

I find this attitude on code reviews to be extremely demotivating, especially on code bases where I am not the primary committer or lack overall responsibility

by sambe 10y ago

I find this attitude on code reviews to be extremely demotivating, especially on code bases where I am not the primary committer or lack overall responsibility for the system being changed. It completely distracts from the code under review, and it only requires a small amount of social/organisational ill health to be abused. Even if you believe that the next committer should clean up, it can require quite a big context switch - either the clean up explodes into quite a big task and you have to question the trade-offs being made in doing it now, or it doesn't have full focus and becomes risky and/or no real improvement. I don't happen to believe that the next committer should clean up in most of these situations. Minor things, sure, you should do it (but don't side-track the review with it!). In my experience, people who already couldn't be bothered to do it or clean up later use a new commit as an excuse to get their work done for them. I've often seen people criticising tangential code issues in review who were responsible for the original commit. The question becomes - if it's such a minor issue for me to clean up, why wasn't it a minor issue for you to clean up? If it's bigger, how did it pass review in the first place? My view is that extraneous cleanup should be proportional to the original change at most - and not discussed in code review of unrelated changes. If a particular area of the code sees sufficient churn to accumulate more and more debt, everybody will know and talk about it, it will be clear that some specific refactoring is necessary, and more importantly it will be evidence that small cleanup is worthless (high churn) and a better architecture is valuable (high churn). A given team may have high tolerance for technical debt and thus never do this work. But then they are screwed anyway, why create noise at every review just hoping that someone else solves it? Remember, there's also a good chance that the mess stays the same as it always was, sitting there passively. And there's a chance that the cleanup introduces unanticipated problems, especially if it's legacy code with no/less testing.

justinlaster 10y ago
Are you addressing my statement or someone else's? I don't understand how going through with code reviews for any change is somehow distracting from "code under review." And surely addressing the code you're touching isn't "extraneous" cleanup. You're modifying the code, you can do it bit by bit.
- nostrademons 10y ago
  I think he's addressing this line from the original article, which sparked this whole sub-thread way up-thread: "Also, there are 2 old Debug commands, an unassigned variable warning message, and a hard-coded Employee ID that will all have to be fixed before this module can be moved to production." Basically, it's the idea that if you touch a piece of code, you have to fix everything else wrong with it, even if that involves touching other areas of the code that you weren't looking at initially and may not even have familiarity with. This is "the perfect is the enemy of the good", applied to code refactoring. If your policies put people on the hook for fixing mistakes that were made by other people a long time ago every time they touch code, there is a very strong incentive to never touch code. You get a bunch of hyperactive software wizards that shit out a working product (with a bunch of crap code), put it on their resume, and then move on to another employer. The people hired after them have a strong incentive to never do anything (because touching code will require that they touch a bunch of other stuff that they don't understand and have no interest in dealing with), and so they browse Reddit and Hacker News all day long. The code remains crappy, but its crappiness can't be blamed on anyone except folks who are no longer with the company. Actually, this is a pretty accurate description of the software industry.

by sambe 10y ago

justinlaster 10y ago

Are you addressing my statement or someone else's? I don't understand how going through with code reviews for any change is somehow distracting from "code under review." And surely addressing the code you're touching isn't "extraneous" cleanup. You're modifying the code, you can do it bit by bit.

nostrademons 10y ago
I think he's addressing this line from the original article, which sparked this whole sub-thread way up-thread: "Also, there are 2 old Debug commands, an unassigned variable warning message, and a hard-coded Employee ID that will all have to be fixed before this module can be moved to production." Basically, it's the idea that if you touch a piece of code, you have to fix everything else wrong with it, even if that involves touching other areas of the code that you weren't looking at initially and may not even have familiarity with. This is "the perfect is the enemy of the good", applied to code refactoring. If your policies put people on the hook for fixing mistakes that were made by other people a long time ago every time they touch code, there is a very strong incentive to never touch code. You get a bunch of hyperactive software wizards that shit out a working product (with a bunch of crap code), put it on their resume, and then move on to another employer. The people hired after them have a strong incentive to never do anything (because touching code will require that they touch a bunch of other stuff that they don't understand and have no interest in dealing with), and so they browse Reddit and Hacker News all day long. The code remains crappy, but its crappiness can't be blamed on anyone except folks who are no longer with the company. Actually, this is a pretty accurate description of the software industry.