Recently, I was refactoring some legacy code that was incredibly complex (integer overflow on the N-path complexity score) and with no tests around it. It also was a significant part of the product that provided the core value to our customers. We wanted to decompose a big fat controller into some smaller logical services so we could reason with what was happening and migrate some components to reuse other parts of the system. Even though it was fairly trivial work, it can easily be seen as a risky activity and required a pretty thorough code review.
When I talked to my teammates about how I was going to go about doing it, we essentially built a list of checks and different steps I should go through to achieve the outcome.
With this process of sorts, I set out on my task, but paid a particular attention to using the Git history as a detailed log of the steps I took during the process.
Each commit of the pull request was then simple (one type of change) and descriptive, such as:
- Step 1: Copy function body from controller to a service without change
- Step 2: Replace injecting entire container with explicit service injection
Some steps had diffs of thousand of lines and some were only a handful of one line changes — but each diff was completely described by the commit message.
This may seem intuitive but it’s very easy to fall back to the tendency to lump lots of small/medium changes together or only commit when you’ve made a certain size code change.
When you think about this from the perspective of those people who will be reviewing your code, it’s very difficult to read a diff where you may have moved some code and made slight modifications to it, for example. Plus, it just downright sucks wafting through thousands of lines of code.
Your commit messages for each commit of a pull request greatly helps the reviewer understand what changes you’ve made and demonstrates the steps you took to get to there. As we squash-and-merge pull requests, the individual commit messages are then joined together into the final commit message body.
As a result, the reviewer said it was the easiest PR they’ve read, we quickly fixed up a few things we found in the review, shipped the changes without defects and continued on.