Lately I have been focusing upon subtly improving the way I commit my work to the repository, trying to write a story with my commits.
New team, new procedures
Not too long ago I transferred to a different development team within my company, and besides the different social culture I also encountered a distinctly different technical culture. I think this is mostly due to two things.
First, my new team lacks a dedicated tester, making each developer ultimately even more responsible for not only their own code, but also for the code that they test from the other developers. Second, the team is responsible for one of the real back bones of the product. In other words, mistakes tend to become showstopper problems that disqualify builds for use.
So, to make sure that the quality of our work is up to snuff, the team has introduced a mandatory peer review step before code is even considered ready for testing. This means that another developer of the team, who wasn’t involved in creating the story, has to sign off on the code as if it was his own. This has the added benefits of knowledge sharing as a side effect!
Going into the transfer to this team, I wasn’t really worried about my code being reviewed. Additionally, reviewing other people’s code is proving to be a great learning experience. But lately I have started to notice that maybe I could have made life a little easier for my colleagues.
How to make a reviewer happy
At first, I reviewed code by compressing all the changes together, trying to see what it did as a whole. Although this has some merit for getting an overview, seeing all the details is too overwhelming for anything but the most trivial change.
Reviewing one changeset at a time does allow you to see the details; the ‘how’ and the ‘why’. However, you are still dependent on the author to make it easy to understand their thinking and spot problems.
Each changeset is about one idea
If a changeset does one thing, e.g. renaming a method, changing a method signature, refactoring an algorithm or adding a piece of functionality, it becomes much easier to verify that this is indeed what has been done and that it has been done correctly and completely. Even refactorings that touch many files can be quickly verified by scanning for unexpected changes in the pattern.
No broken changesets
Looking at code gives a lot of insight, but sometimes it can be a struggle to figure out what the code is meant to do. Updating to a changeset and running the software allows a reviewer to relate the change in code to the reality in the product. You may even be able to catch a mistake this way and pinpoint it to that exact changeset.
So no excuses, even though you may commit several changesets at once and the end result will run fine. It is a great boon for reviewing (at the very least) to have a working product, no matter which changeset you update to.
Changesets have a logical progression
If changesets follow each other logically it is easier to keep track of what the author is thinking. For example, nestling a signature change that isn’t necessary for implementing the feature between changesets that are, will leave a reviewer questioning whether the signature change is significant.
Instead, try to write a story with your changesets. Create an introduction with preparatory changesets, take the reader on a journey with the changes that implement the feature, write a strong ending with changes that are possible now the actual change is in place, and finish with an epilogue of cleanup.
Telling a story
This doesn’t mean you should start working this way! Simply because, most of the time, what needs to happen will reveal itself organically while working on the changes. And I’m definitely not opposed to a bit of ‘squirrel! coding‘, i.e. getting sidetracked by code you just ‘have to’ fix, although you may want to commit whenever you sniff that squirrel!
For example, I was working on replacing a block of form elements. I had already replaced several of them before, so there was a bit of convenience, but I didn’t know what kind of form elements this block contained or how they worked. So I took it one at a time, creating a bunch of changesets that looked something like this:
1. Converting first few text fields 2. Converting dropdown field 3. Converting datetime field with checkbox 4. Converting datetime field 5. Converting dropdown field 6. Adding dropdown field to convenience 8. Adding datetime field to convenience and reordering method parameters there 9. Converting another datetime field 10. Converting last couple of text fields 11. Fixing bug in dropdown field convenience
As you can see, this results in a story structure a six-year-old would be ashamed of, but, assuming you are using a DVCS, you can massage the structure at any point before publishing. By reordering, splitting and merging changesets, the story that the changesets eventually told went more like this:
1. Adding dropdown field convenience 2. Adding datetime field convenience 3. Converting form block 4. Reordering parameters in convenience methods
Now you can be sure that each changeset is about one thing, the software works after each one, and there is a logical structure. Your changes should now be much easier to follow, also for yourself. Ultimately, the biggest reward is when you expose a mistake you made before you submit your changes for review!