Doug Turnbull
Category: Uncategorized
Something amazing happened today on our Quepid project. We did a code review.
Not some formal thing whereby we sat together in a conference room with someone presenting hundreds of changes trying to get them approved as we all silently nodded. We of course, don’t work that way. We prefer to use git flow. Developers on our distributed team work on issues or features in a branch, issuing a pull request once they are satisfied with their changes.
For a few months, I’d become the Linus Torvalds of this whole process for Quepid. As pull requests came in, I peered over them from on high. I wanted to understand everything. So I dug into these pull requests with great enthusiasm commenting freely as I went along. Commenting too when I saw something from fellow developers have confused me or left me concerned. Not one to hide my opinion, I spoke every time I saw something.
The Internet absolutely sucks for anything that even has the hint of possible conflict. So even though I’m far nicer than Linus I still have my tastes, opinions, and a strong sense of ownership over Quepid. Commenting on other’s work with curt comments on the Internet is a magnet for conflict. It became easy for others to take my thoughts more personally than I expected. At one point Eric Pugh commented that there seemed to be a lot of conflict in the pull requests. I thought – I don’t think so! We’re just having a frank conversation about this code!
Yet fellow developers would joke – “sheesh don’t give Doug those short pull requests. He really digs in! He’s so picky!” Moreover, I was often increasingly holding back on my thoughts. Minor suggestions without any particular action item, stray observations, or purely educational points were being left out. My comments too sometimes came to a lack of understanding. Simply perusing the code in the pull request left me with questions and didn’t help me truly understand the intricacies of the problem solved. So many times my questions, complaints, and concerns were out of ignorance.
Today, I decided to do something different to take the conflict out of the conversation and help me really understand the problems being solved. Something I’d done dozens of times in non-distributed teams that didn’t use git flow. Receiving a pull request, I had the completely unoriginal idea to ask my colleague – “do you want to just jump on a 15 minute hangout to go over this PR?”. In other words “do a code review!?!”
What followed was by orders of magnitude both the best code review and best pull request review I’ve ever done. In previous lives, code reviews were giant endeavours that happened far too late to get value. Pull requests were just the right size for a good review. Yet pull requests are a thing to talk about on the Internet – essentially a solicitation for criticism. Its easy even on a distributed team where everyone knows each other to misinterpret critique for annoyed criticism. By chatting in person, the team understands that my questions come out of appreciation for their work and out of our shared sense of care for the product – not a “get off my lawn” tone that can often be heard when reading comments from the Internet.
During our code review we walked through the code. We TALKED about the code. Which means we were both much more engaged than the scanning that fills in for most pull request reviews. We BOTH felt like we owned the code, including any problems. Our mutual understanding for the work done and the code landscape ran deep. During this time we together managed to:
- Discover 2 minor bugs in the implementation
- Identify patterns I liked in their code, encouraging them in the future
- Demonstrate to the developer an important Angular testing technique which will be useful to them in the future
- Teach me more about the considerations that they thought about when implementing the pull request – giving me a deeper appreciation for the code and problem
- Make longer term plans for code outside the current issue, including our preferences around things like database model objects and controller responsibilities, laying the groundwork for future similar work
As we talked, we commented in the pull request with takeaways from our conversation. Instead of comments being the start of a conversation predicated on conflict, they were the result of two minds meeting from different points of view at a shared solution.
In other words, pull requests by themselves are a barely acceptable code review gruel. Pull requests without in person code review might even be a bad smell for me from now on. Reviewing just the pull requests can create bad feelings very easily. They don’t work particularly well either. Code reviews done in person trigger numerous important conversations that don’t happen without being engaged in code. More importantly, when there is an area for conflict or a possible bug, you can work on it together instead of throwing the problem back-and-forth over the fence possibly blaming and complaining.
Code reviews puts the code ownership in the hands of the team. Pull requests can be an invitation for the blame game. I can’t say I’ll do a code review on every pull requests, but I certainly will for anything non-trivial.