Think of it like asking a high school student if her English papers need editing. Most likely she will say no, but most of us know the answer is yes.
Code is like a rough draft of a paper. It needs to be edited and reworked. As hard as it is to admit, we can’t do it by ourselves. We as the code authors can’t review our code by ourselves. We need help … and that’s ok.
Why do we code review? Here are the four main reasons:
Like I mentioned above, code needs to be reviewed to find mistakes (logical, standards, design, etc.). One of the most beneficial reasons to do a code review is to improve quality, in all aspects of that word: quality of the code, the project, the user experience, etc.
Another reason to do code reviews is for knowledge sharing. When multiple people review the code, now there are multiple people who know what that code does. This makes it much easier to cut down silos and reduce the need for code heroes (a code hero is where only one person knows the code and thus has to be relied on to save everyone when there is an issue in that code).
The 3rd most important reason for code reviews is to enforce code standards. It is always best to automate as much as you can to enforce code standards, but not every standard can be automated. By enforcing the standards in a code review, we help ensure the code is similar / readable / maintainable by anyone on the team.
It also leads to the 4th main reason why we do code reviews: training apprentices. Because the apprenticeship program is so integral to who we are at EduSource, we heavily rely on code reviews to train our apprentices – teach them how to (or how not to) write good code / solve problems / use good architecture. Some people wonder if code review is too late in the process to catch these issues. It’s not for us, because of how we break our work into small chunks (more on that in the next section).
How do we do our code reviews? There are several key factors that influence the effectiveness of our code reviews.
First, we perform them on a daily basis. Actually, it is not necessarily daily, but it is as needed (pro re nata for us Latin nerds). We make it a priority that every User Story we work on, every line of code that is written (by apprentice, senior devs, or even CEOs), is code reviewed. We try and break down our work into meaningful small chunks of work, and then code review that work. That means within a given week, most developers are submitting multiple code changesets for review.
One of the reasons that allows us to do daily code reviews is that we do Tool-based reviews. We actually use the Atlassian suite of products (Jira and Crucible for this process). So, with the click of a button, a developer can create a code review from a User Story with all of her changes highlighted.
Crucible assigns the right people to the code review and allows us to create Change Items in Jira (our issue tracking system) whenever we see something that needs changed. One of the nicest features of Crucible is the ease in which a re-review can happen. After code review issues have been fixed, a reviewer will re-open the code review. Just the fixes are highlighted so that the reviewer can just review the new changes and sees them inline with the old code and code review comments. This tool makes it very easy for us to do code reviews from multiple locations and time zones.
Since we do a Group code review, we have found it very beneficial to also include a checklist of what to look for in a code review. Each person on the code review takes a specific lens (Standards, UX, Unit Testing, Completeness, Security, or Architectural). The lens focuses the reviewer’s attention on a checklist of things. This helps avoid Social Psychological issues like: Social Loafing (combatted because reviewers know they are the only one with that lens, so they need to do a good job); Inattentional Blindness (no longer do reviewers need to look for everything, but they can use Selective Attention and just pay attention to the important things on their checklist); etc.
The lens that is taken also helps set an expectation for how much time that person should spend on the code review. For EduSource, our code reviews became much more efficient and effective when we started using these checklists.
Important things to remember:
Code is not tied to a person’s identity. Tearing down someone’s code does not mean we are tearing down the person. This can be difficult at first, but in an open, candor environment, people do learn this lesson.
That being said, we try not to make code review comments personal (i.e.: “Why would anyone ever do this!”) but rather word most of our comments in question form, since we often don’t know everything that went into making a decision (i.e.: “Have you considered using this pattern here?”)
Everyone gets code reviewed! Our first day apprentices get reviewed. We often tell our apprentices that they will get a lot of code review comments in their first few code reviews. But hopefully, as they learn our processes and standards, they will receive less general comments on their code reviews.
We also review our senior developers’ code. Often it can be intimidating for our apprentices to log comments or questions on a sr. dev code review, but we tell them it is part of our culture and it is how we all learn and make ourselves better. We even review code written by our president. Jason’s most recent code review had 95 issues with over 70 issues that were required to be fixed. To be fair, he wasn’t current with our latest standards when he took on that project, but he sure is now!
Lastly, as the initial author, always do the first pass on your own code review. Hopefully, you will catch anything that is obvious and can fix it before others put their time on it. Also, if there is confusing code, or a section that you know will get lots of comments, add a note from the author as to why you did it the way you did.