If you’re reading this, it is most likely that you are a software developer.
And as a knowledgeable software developer in 2018, you probably host your code on GitHub or Gitlab and use a well know git workflow like http://nvie.com/posts/a-successful-git-branching-model/. And that’s great.
Since it’s 2018 you’re probably a CI/CD believer and your daily routine includes git branch -b feature/my-cool-featureand the words “pull request” are in top ten expressions you use when communicating with your fellow SD peers. The other nine are very likely to be “it’s a mess”, “that’s my sandwich”, “lunch, anyone?”, and “waiting for a review”.
The Pull Request
Let’s talk about the lifecycle of a PR.
A product manager comes up with a great idea “We need more gifs in our app”. Of course, after a short meeting, the PM gets a bonus for this amazing idea and you get a beautiful ticket, with all the specifications you’ll need, “ADD JIFFS!”. Who needs more?
Now that the task is on your table, you get to work. Branch out from master, write some shiny clean code and push your branch to GitHub. Then something magical happens.
You’ll be prompted to compare your changes with the master and create a new PR. Two clicks later, a PR is open, and you just hope that colleague that you don’t really like won’t pick it up and review it.
The Review
Great! Now your code is checked in, it was all nice and easy and now the awesome new feature is off your table and on the dev team’s waiting for the review. You glance on GitHub’s PRs page
23 open pull requests
Oh, this might take a while…
The truth is, that the average lifespan of a PR, from creation to merge is a bajillion* days. For an Agile team, that’s not very Agile. Every day a PR is just waiting for a review, is a day without gifs for your users. And nobody should have to live without gifs.
Now that you know how harmful a late PR can be, what actions can you take to improve that?
Non-blocking PR reviews
Meet Chad**. Chad is a SD and he just opened a PR. But, instead of just waiting for somebody to make some review time, he decides assign a reviewer himself and then merge the PR, so that it can further be tested and deployed. YOLO!
He’s merging a un-reviewed PR *gasp* ???
Yes, he is.
Because Chad is Agile to the end and he knows that delivering features to the users is more important than getting the code style right. The hero of gifs they called him.
Later that week, Chad’s colleague, Tanner** goes ahead and actually looks at that PR. He grunts and scratches his long brown beard. He really hates “helper” classes, and right he is to do so. He then comments “I think all helpers should go and commit code seppuku. Please change that”.
Chad reads the comments, rolls his eyes, and makes a note to change that in his next PR. Also, he adds one more note to watch a samurai movie asap.
#SoAgile
OK, so you’re still hung up on that merging without code review thingy. Well, if delivery being more important than code standards did not convince you, here’s a couple of other reasons that might do it for you.
- Code reviews are not as relevant as you might think. Look back on the changes that were requested by or to you. In 90% of the cases they are complaints on code style and conventions. You don’t see too many comments in the lines “that piece of code will cause a memory leak and the app will crash and life as we know it will come to an end” and that because a review is someone reading code in between his actual tasks and not a deep dive in it.
- The longer a PR stays in waiting for review mode, the greater the chances of it becoming deprecated. I’ve witnessed many times, PRs that were kept in debate for so long that eventually they just got closed because the feature was no longer relevant. In a fast evolving business, you must deliver required features fast. If you can’t do that, then you’re affecting the business. Waisted time, waisted business opportunity.
- “Review Me!” pressure. Pending PRs will be mentioned in the daily standup, again and again, until at some point the assigned reviewer will just be annoyed by it and do it because he has to, not because he believes in the true and pure power of code review.
The Renaissance of Code Review
So far we talked about why blocking code reviews are bad and why non-blocking code reviews are good. So that’s settled.
But what can I do to improve even further this process?
Glad you asked, here’s a couple a things that will help you get to the next level.
Pair programming review. Instead of reviewing the code and passive-aggressively commenting on GitHub, why not just sitting down, the PR OG and the reviewer and do the changes as they are requested. This is far more effective and who knows, at the end of the day you might find out that Tanner’s not only a grumpy bearded dude that does not like helper classes, he also likes cats, has a fedora collection and that he does not like helper classes because they smell of laziness and evil spirits. Ok, might have let myself get carried away with the clichés over here, but bear with me.
Reserve time in your schedule for reviewing. Either code review or applying requested changes, reserve some time every week and do it properly.
As a conclusion, if your team does blocking PR review, take some time and check if that fits your process or if it just slows you down. If it does, try non-blocking PRs, see what happens.
I’ll let you sit on this.
Notes
- * Made up numbers that prove my point.
- ** Yes, I’ve googled “whitest American names”
