Writing code is a lot like maintaining a Bonsai Tree. If you stop pruning it it’ll stop being a Bonsai and turn into a bush. Little tweaks, frequently aesthetic ones, will help to keep it beautiful and under control. It will still grow in unexpected directions, as other developers make changes, but careful pruning will keep it balanced, and healthy.
What is “careful pruning” then?
Each file is a branch on our tree. The methods, are leaves. We come in to work and start examining one of the branches. Some days we need to encourage it to grow a specific way by adding a feature. Some days we make little snips to correct a bug. But what happens if, upon examining your branch for other reasons, you discover that it’s grown into spirals and knots. You can’t reach your clippers in to snip the leaves you need. You can’t do much of anything without difficulty and frustration, and frankly, it looks like crap. The unexpected spirals and knots need to go.
This, however, is the point where you’ll hear other developers saying “No! Don’t do that!” They’re upset, because your task wasn’t related to the knots and spirals. You were just there to trim some leaves. If you go and get rid of the “other” problems then how will they be able to tell where you trimmed?
To abandon the metaphor for a minute, they’re concerned that the diffs will be so filled with aesthetic changes that they can’t see the changes that were relevant to the bug fix, or feature add. And, they have a point. Being able to look back and understand the changes another developer made is a very valuable tool. But some developers get so obsessed with this idea, or so beaten down by others who are obsessed with it that they say things like this,
“The rule for controlled development is that if the line is not relevant to the fix, you do not modify it. If we want to go through and fix all the lines in our product that are over 80 characters, then we log a bug for it.” - A rule you shouldn’t follow
This solution will never leave us with a beautiful tree. I don’t know of any professional developer who has the time to deal with tickets that are that low priority, and no-one wants to anyway.
If you accept that there is value in making the line / file readable, then you must also accept that there will be change unrelated to any bug fix or feature request. You must also accept, that if those changes get put off for “later” our tree will become a bush, and require so much work that it will be almost unrecognizable the day after the change.
So, how do we balance these two concerns? How do we make readability changes to our code without obscuring the functional changes? My recommendations are as follows.
- It should be acceptable for whatever method you’re directly trying to enhance or fix to be refactored for readability. Even if you’re refactoring a 20 line method, for a one line fix. To do otherwise is like having a child make their bed but leave their clothes and toys strewn about the floor. Don’t do a half-assed job. Leave the method better than you found it.
- Separate other aesthetic changes out into separate commits. It would be nicer if you could commit the fix, then commit the aesthetic changes, but we frequently need to make the aesthetic changes in order to make the code readable enough to understand it. So, make it readable, but leave it broken. Commit that change. Then fix it and commit that change.
The downside to 2 is that you have to instruct reviewers to review specific commits, rather than the changeset as a whole. So, an alternative is to make the aesthetic changes, create a pull request / request review of your branch to be merged, and then, once that’s done create another pull request for the bug fix. I think it’s a judgement call, and the appropriate choice depends on the situation.
As per usual, if you’ve decent test coverage everything becomes much easier. Code reviewing the aesthetic commits can be just a matter of seeing if the tests pass, and giving it a quick glance to make sure the other developer didn’t go all crazy-pants with their changes, like changing the indentation style to something no-other files use. You don’t have to stop and consider the funcional impact of the changes, because there shouldn’t be any. “Yup, still compiles, still passes the tests, looks more readable. Approved!”
Codebases are like Bonsai Trees. Regular aesthetic changes are as important a part of keeping your codebase healthy as regular refactorings. Some trivial changes to how you commit your work can elimanate the diffing problems that arise when combining fixes / features with aesthetic changes.