Tagged: Best Practices

On Code Reviews

Or, How I Learned to Stop Worrying and Love the Squirrel

kihroxvh_400x400

In a couple weeks I will celebrate 7 years of professional development. In those years, I have committed code to production in a variety of ways. I have SFTP’d files up to a live server. I have merged feature branches to a master branch. I have committed directly to master in a CI/CD workflow. I have participated in several iterations on git flow style branching. I have rebased live history, sometimes to great effect (removing an excessively large data file from our code’s history), and sometimes to great pain (having to manually rebase a dozen different branches to remove a couple commits that weren’t ready for prime time).

For the first several years of development, these code commits were largely autonomous and lacking any significant review. If they were reviewed, it was only to make sure the new code worked as expected and didn’t have any hidden problems, because we weren’t doing automated testing either. I was growing in skill by sheer attrition, sometimes debugging issues in production, sometimes spending days trying to reproduce issues locally, but only getting fundament development guidance from the occasional blog post or podcast.

In the past couple years, it has been a different story. My previous job was at ePublishing, an all-remote team with excellent developers and well defined standards and practices. It was here I was first shown what a pull request could be: with useful feedback on best practices and standards and multiple people participating and eventually signing off on each review. One rule was made very clear: on any non-trivial change, two :shipit: 🐿s (or :+1: 👍s) were required. This made the call on what was approved very easy to tell, though it sometimes left the PR discussion a little lacking.

A year ago today (8/3) I started working for Ramsey Solutions, a company that does “work that matters” in areas like personal finance and business leadership. This is a great company with great team members, but when I started I noticed some gaps in the code review strategy (or lack thereof). I was back to the wild west of optional reviews, and fast/shallow analysis of the code when it did get reviewed. So I resolved that (at least on my team) things were going to be better. With the help of my leader our team started using the two squirrels strategy I was used to on all our PRs going forward. This helped quite a bit on it’s own, because it got more people reading the reviews, which in turn gave more chances for discussion. But as the team grew, we didn’t have any consistency with the amount or severity of feedback we were giving besides the final :shipit:. In addition, we sometimes were giving “half-shipits” via the :+1:, which led some to wonder whether the code was good to go or not. Several of us on the team also started inserting things like “⛵️-it” or “🐏-it”, which, while amusing to punny people like me, really confused our newer team members.

A few months into this practice, at a quarterly team planning session, we discussed “the PR problem” and eventually codified some new rules and concepts.

Firstly, we decided that you could no longer leave :+1: (or anything else, no matter how punny) as a final meaningful response on a PR. If you are going to participate in the code review at all, you must drive the PR forward until you feel comfortable with a :shipit:. To this end, you might need to add tickets to the backlog to fix things that could not be addressed in this PR. These tickets are new technical debt for the project and should be remedied as quickly as possible so that paying down the the interest doesn’t overwhelm new work.

Secondly, we decided to call out (literally, label) four severity levels of comments you can leave, and we clearly defined each level:

  • The :shipit: is the first and most obvious severity level and means you are 100% OK with this code being in production as-is.
  • The next severity is blocking which, as its name implies, points out something that prevents the author from giving a :shipit: until resolved (either in this PR or as a new ticket). These can range in cause anywhere from critical logic issues to coding/design standards violations. As long as you won’t give approval until it is resolved, it is blocking.
  • The middle severity is rocking. These are the “nice to haves”, things like refactoring messy code or following a standard that is not officially codified, or less important.
  • Note: If you only give rocking feedback or below, you should also give a :shipit:, because you didn’t find any blocking problems. The committer should read all these comments and at least consider whether now is a good time to make the change, or even if they agree with it (discussion is encouraged!). These comments can also be sources of tech-debt tickets if the committer agrees the change should be made, just not right now.
  • The lowest severity comment is talking. These are questions and general comments on the code, including :+1: as praise for a particularly well written line of code (or refactor/deletion of a poorly written one!). These often require no action unless there is a question attached, but should still be reviewed before merging.

Third and finally, we started leaving comments on specific lines of code wherever possible so that when the comment was addressed the discussion disappeared, hopefully leaving a fairly clean diff at the end (:shipit:s being the one kind of comment that is encouraged only at the top level).

After this first draft of rules was decided at our quarterly planning session, the quality of our code reviews went up drastically. Everyone committing code knew exactly what to expect when they submitted their PR, and conversations were much more focused because the original commenter gave their feedback a reasonably severity level. A lot of the discussion around rocking and talking comments has helped us to codify new standards/guidelines for our code base, where as the non-committal :+1: which might have been given before would often fail to generate a conversation.

After we had been using these conventions internally for a while (and as our team grew) we started using them when giving feedback on code reviews outside our immediate team. This caused some confusion initially, so one of our team’s developers gave a short talk about the convention at the next technology team meeting (our team is about 8 people, but there are over 80 people in the technology space here). Other teams then started adopted the standard, some of them replacing the English terms of blocking, rocking, and talking with emoji for brevity: 🚫, 🎸 (or 🤘), and 💬. Our team had actually discussed this but decided against it at the quarterly meeting because it seemed too finicky, but more power to the teams that decided to use them as long as they are consistent and clear.

I have really appreciated the level of feedback we have on code reviews now, and of course we are always looking for more methods to improve the experience even more. But discussing processes like this one with your team, even if you think you are doing everything all right, can be very instructive because the discussion itself can help further define things even if no actual changes are needed.

So go out (or possibly stay in) and improve your workplace environment today!


 

PS: After several months of using the new conventions, our team had a chance to work on a large project under a tight deadline. Since we were spread so thin, our fearless leader (er, our Director of Technology) decided to take a hand in approving PRs. He declared by fiat that his :shipit: squirrels were worth two of ours, so we lovingly created a special :leadershipit: emoji on Slack just for him.

773a89c1682f2c37