Should I also change this while I'm at it ?
I recently got involved in a disagreement between two developers. One was reviewing some code changes that the other had authored. The reviewer requested some additional changes that were not necessary to implement the current feature, but would be a welcome improvement to the maintainability of the code.
I took the opportunity to put words on two conflicting principles that I try to follow when I’m asking myself if I should include some additional changes in a unit of changes (commit, pull request, etc).
- Follow the “scout principle” ("Always leave the campground cleaner than you found it"): whenever it’s adequate, improve the code that you’re modifying. The focus is always to improve the maintainability of the code base in the vicinity of the changes we’re making. We usually already have a very clear view of how that part of the code works, since we had to understand it to make the actually necessary changes: it’s often cheap and safe to include some other improvements with them.
- Keep merge requests small: Focus on one thing for each MR, keep it as simple and small as possible, make it easy to review. The more code we change, the riskier it gets. Errors can creep in. It gets harder to understand what we changed, to make sure that it’s sound.
Those two are contradictory; that’s a good thing:
- the first one helps us progressively improve the code, but it could lead us down a rabbit hole of changes that are actually not needed at a given time. It delays the feedback loop.
- the second one helps us be efficient in the short term, and helps us stay focused on the changes we really need to do. But it could lead to a messy code base because we apply changes without taking a step back. It can also lead to missed opportunities of low-cost improvements that are right before our eyes.
So those two principles work well together, each keeping the other in check.
Here are examples of questions I ask myself when I need to decide:
- If I include this code change, how far away am I from my original objective ?
- Should I worry about the “big picture” of my changes more ? Is the code base still easily maintainable after my changes ?
- Am I making my merge request harder to review ? Is it going to take longer to merge this because of those optional changes ?
- Am I adding a risk of unreliability with my additional changes ?
- Should I do some other changes to maintain the coherence/consistency of the code base, rather than do only my changes and suffer the inconsistency ?