3 ms·
Often people suggest rewriting a method because it's so complex and poorly understood that it can't be maintained as-is, and for a method like that, you're righ
by dkarl 9y ago
Often people suggest rewriting a method because it's so complex and poorly understood that it can't be maintained as-is, and for a method like that, you're right, it probably can't be rewritten without inadvertently changing the behavior.
I think what is being proposed here is rewriting methods that people usually don't consider because they're basically okay. Maybe the name is a little weird now because the usage of the method has changed. Maybe two methods that used to do something different now do effectively the same thing. But everything is basically correct and readable, and people don't want to make a bigger change than necessary.
Personally, I think I lean towards more aggressive rewriting than other people do. I try to make sure names make sense. When special cases are removed I check to see if this allows me to make the rest of the code simpler. When I see the same thing being accomplished in different ways in the same file, I'll take a minute to make it consistent, so that same things look the same.
But people see this as a trade-off, especially when it comes to code review. Several times when I've made major changes that needed to be code reviewed, I've taken extra time to re-order my commits so the change can be reviewed in two steps, first reviewing the refactoring that was done and then reviewing the change that was made. And with a single exception nobody has taken me up on it; they've insisted on reviewing the entire change at once. Instead of a straightforward refactoring and a straightforward change in functionality, now they're trying to make sense of a combination of behavior-preserving and behavior-altering changes. Because code gets reviewed that way, people tend to refactor less than is optimal, because they want to give their coworkers small diffs that highlight the logical change that was made. This is a tendency that can lead to a gradual accumulation of history in the form of duplicated code and nonsensical names, which I think is what the "rewrite every time" rule is meant to counteract. A technical rule won't make people forget a social trade-off, but it might help them remember the other side of the trade-off.
- Reefersleep 9y agoDo two pull requests: one pure refactor, one pure logical change. I've been doing this at work, and it's great!