It is said that in order to become a better writer, one should read a lot. In a related way, one of the best ways to get better at writing code is to read a lot of code. Not only is it a great way for you to level up your own skills, but you can help improve your team and your code as well.

What Is Code Review

Code review can come in a number of different shapes and sizes. Code review can happen from having programs evaluate your code. It also includes when you look at someone else’s code or when someone else looks at your code.

You may be familiar with some of the common automated code review programs. If you’re using an IDE like PHP Storm, you may have some these running all the time. Any sort of analysis of problems or potential problems on your code can count as automated code review. Syntax checking is a form of automated code review. This can come in the form of a squiggly red line indicating that there’s a problem in your code that will prevent it from executing, or indeed, from even compiling. If you’re running with strict types enabled, your IDE may be able to tell you about other problems like passing incorrect parameters or returning the wrong values from a method that includes a return type. Even if you’re not using strict types, but are using doc block comments, many IDEs can help out as well.

PHP Codesniffer is a program that can be used to determine if your code conforms to your code formatting standards. In certain circumstances, I think I could argue that executing tests against your code could also count as a form of code review. There are other options as well, such as the commercial Code Climate. If you’re interested in other static analysis tools, there’s a large curated list available at https://github.com/exakat/php-static-analysis-tools.

Let’s talk about some common ways that code review is done by and for humans though, and why many of those ways don’t work for improving code quality.

Email Code and Diffs

One common way to perform code review is to email around code or code diffs. After reviewing the code, any comments or suggested changes can be emailed back to the original developer. I did this for a number of years but it’s not terribly effective. First of all, emails are often ignored. Secondly, I’ve found that humans tend to interpret written communication, especially written communication that may be critical of something they’ve done, in the most negative way possible. This seems to be true not only for code reviews but in general. Comments indicating that something may be done in a better way, or that perhaps the provided code doesn’t cover an edge case or may be less than optimal in a number of ways can be read as “your code sucks”. Depending on the developer reading the comments, and the level they associate their own self-worth with the code they write, the reaction can be anything from apathy to anger or self-loathing to wanting to re-evaluate their career of choice.

In the best of cases, the email may be taken in stride or even potentially ignored. If code review comments are ignored regularly, it may lead to the developers deciding that code review is a waste of time. Why should they bother to comment on code when their comments are ignored? Why try to make the software better? Eventually, this leads to the whole process being abandoned with the software heading down a shame spiral of code rot and tech debt. So let’s talk about another way to do code review.

Evaluating Diffs in a Repo

With modern repositories like Github, Bitbucket, and Gitlab, both hosted and self-hosted, this method of review is actually less problematic than it was just a few years ago. Nowadays, many repositories allow for code review in the repo, but some still do not. In those rare cases, it means that someone is looking at the code in the repo. This is done typically by a single developer, probably one of the more senior members of the team. It doesn’t work if there’s very much activity in the repository, so it typically only works if it’s on a small team. And since only one person is doing the code review, it requires diligence and a commitment to ensure issues don’t slip by. This could make it difficult for the reviewer to take time off work which leads to burnout. Commenting about changes in repositories like this is often done via email. Please see the previous section for problems with code review via email.

Even with many modern repositories, if the development team is not following a pull request workflow for changes, the repository may not provide any, or at least not any good ways to comment on changes. If the repository provides for pull requests (or merge requests in some repositories’ nomenclature) then it typically provides for a way to comment on the code in the pull request. However, even if comments can be made on code that is not part of a pull request (perhaps comments made directly against a commit), there is also typically no good way to notify the developer of any issues or questions found, and little incentive to work on fixing those issues since the code has already been integrated into the main code line.

Group Code Review

Another common way of providing code review is done when a group of developers gathers around a television or a projector in a conference room to review code. This can be a single developer’s code or all the code changed during a particular time period, or anything in between. There’s a number of problems with this approach. First of all, it’s another meeting, which means it’s time that developers are not able to think or build software. It takes up a lot of time, and that time is multiplied by however many developers are in attendance.

If the code review is focused on a single developer’s code, they can often feel like they are being attacked. With all other developers focused on pointing out problems or issues with the code under review, it may be necessary to include someone in the role of moderator to ensure the code review stays productive and that the comments and questions are directed towards the code, towards making the software better, and not against the developer who wrote the code. In these cases, even though everyone in the room can see and interpret body language and signals, it can still be a very rough experience for both the developer and the reviewers.

Additionally, there may need to be someone in the role of the note taker. Since comments and questions are made verbally, someone needs to keep track of what was said and the responses as well as any changes that were deemed necessary by the team. Overall, a group code review is often not a good use of time. It can be effective sometimes, but more often than not it can be a waste of time, and it’s certainly less efficient than other methods.

Over-Shoulder Code Review

This method of code review leaves the original developer in control of the code review. This is typically done with the developer in charge of the mouse and keyboard. The review in this situation is along for the ride as the developer clicks through the code and explains, often very briefly, anything they think may be of interest to the reviewer. There’s typically little chance to really review the code since all that’s provided are fleeting glances and jumping between files. If the developer wants to hide certain areas of code because they know the reviewer wouldn’t like it or because they are not proud of it, then the overall code quality can decrease even if the reviewer is doing their best to try to help.

As the developer is cycling through code, explaining to the reviewer what they are seeing, any time the reviewer needs to ask a question or make a comment they will be interrupting the developer. This can be frustrating for both parties.

If the code is reviewed this way by more than one developer, then the original developer is losing even more time. This tends to lead toward more rushing, more skipped code, and more glossed-over explanations.

Pull Request Code Review

With a distributed version control system (DVCS) like git or mercurial, pull request code review is one of the better options for reviewing code. As mentioned earlier, modern tools and repository hosting options tend to include some level of pull request review capabilities. If you happen to still be using something like CVS or SVN, then your options may be more limited.

With a DVCS, branching, and merging is simple and easy. This leads to a more natural use of a pull request process. For repositories like CVS or SVN, branching and merging are more complicated, more likely to result in merge conflicts which means that a pull request process is less likely. Developers tend to rush to get their code checked in quickly so they don’t have to deal with merge conflicts which means code review is often skipped.

Repository software has definitely improved over the past few years which has included improvements in their ability to perform effective code reviews. For the remainder of this article, my comments will include both these modern repositories and their ability to assist with code reviews as well as other stand-alone software designed specifically to help with code reviews.

Code Review Software

Code review software (including many modern pull request systems) helps immensely with good code review practices. The software is designed to make code review easy and effective. Because it’s software, the reviews can happen when it’s more convenient for the reviewer. With meetings and over-the-shoulder reviews, schedules must be coordinated. With review software, the reviews can happen between efforts to build features or fix bugs. Assuming each reviewer takes the same amount of time to review the code as they would have had in a group review, the code review can be more effective.

Code review software allows for reviewers to make comments on the changes and for other reviewers and the original developer to review these comments, as well as replying or opening issues. Some pull request systems only allow comments to be linked to a single line of code which can lead to some confusion or ambiguity about what the comments are referring to. Better code review software can allow you to select a group of lines and can indicate that the comment is about a block of code. That way if the comment is about more than a single line, it’s very clear. It may not seem like much but any reduction in cognitive load for understanding and resolving code review comments can be very helpful.

Additionally, comments can often be made on the entire changeset as a whole. The ability to reply to comments means that conversations can be held, and recorded between the developer and the reviewers. If your recall, I said that textual communication is often taken in the most negative way possible. In order to combat this, it may be necessary to create ground rules for code reviews. The code reviews must be about the code. No personal attacks are allowed. If the back-and-forth comments and replies grow beyond a certain size, get up and go talk to the developer in person.

On teams I’ve lead, we’ve had to enact all of these. When I started at my current position, code review was not something that was done so I introduced it. In order to keep things civil, we had to come up with these rules so that we could continue to use code review to improve code quality as well as keeping the team working well together.

Approvals And “Ship It”

Not all repositories allow for a “ship it” or an approval, but I think they are necessary. Some teams have made emojis in comments stand for these approvals. The approvals are a reviewer’s way of indicating that they are happy with the code and feel it is ready to go to production. In some cases, there may be caveats, like “Fix this minor thing and then ship it” or “Approved if this thing is done”. In those cases, it’s the reviewer’s way of indicating that the code is nearly good enough and the reviewer feels that the minor changes don’t warrant another review.

In some cases, there may be more than a few issues or the changes requested are complex, or the reviewer feels the developer’s code may need additional review even after the changes. In rare cases, the reviewer may decide that the code submitted for review should not, even with changes, be included into the main code line. Perhaps the code doesn’t meet the original requirements or possibly the requirements have changed so significantly that including the code would move the software in a direction it should not go.

What We Do

On the teams I manage, each group of developers and QA have a set of required approvals that must be in place for the code to merge. We follow a pull request with code review process on all teams, but each team has different requirements. On all teams, we currently require that in order for the code to merge, it must be one commit ahead and zero behind. What this means is that the pull request branch must be based on the latest code in the target branch. Merging the code is done automatically by some software we built that receives events when pull request approvals and builds finish. Each team must have passing builds from the continuous integration server. This means their unit tests and other integration tests must pass for the pull request branch.

For the code review side of things, each team has their own set of rules. For one team that practices continuous deployment, there are two approvals needed for the code to merge. One of the approvals happens when the build finishes successfully. The other approval can come from any of the developers or the testers on the team. Once all those are in place, the code will be merged. We’ve also built in several DSLs or Domain Specific Language that can assist with merging code. If a developer feels strongly that there’s a problem, they can add a comment like “Don’t merge until I approve”. In that case, unless the developer who left the comment has also approved, that code will not be merged. If the original developer or any reviewer wants to make sure another specific developer has reviewed the code, they can leave a comment like “Don’t merge until Dann has approved”.

If a pull request relies on another pull request to be merged first, perhaps in another repo, a comment can be left like “Merge this once has merged” where is a link to the other pull request.

On other teams, the requirements to merge may include more developer approvals and the necessity that someone from the QA team also approves. This allows for developers to review and help improve each other’s code while at the same time allowing the QA team to have control over what is merged and when. We still have some of the review rules in place, but now the review process is healthy and very effective.

You Are Not Your Code

One of the most effective ways of ensuring that our code reviews are effective is to make sure that everyone on the team knows that code reviews happen because we all want to make our products better and improve the software. There are no personal attacks. Developers learn that while they may be proud of what they build, they are not their code. The code we wrote last week or last month or last year will not be as good as the code we write today, and the code we write today will not be as good as the code we build next week, or next month or next year.

I see it most often with junior developers who are just starting out, but also sometimes with senior developers, especially those who have spent a lot of their career working as an individual, rather than as a member of a team. If developers equate the code they produce with their own self-worth then code reviews feel like a critique of them as a person. If another developer is able to suggest improvements to the code then that means the developer is not as good as they could be or as good as they thought they were. This is not a healthy attitude or reaction. For this reason, we make sure that everyone knows we are all trying to help each other improve. The suggestions we make are to make the software better, not to point out a shortcoming in the developer. No matter how much we each know, every other person on the team knows something we do not, and we know something they do not. You are better at something than each of your colleagues, and they are better than you at something as well. It does not matter how junior or senior you are on the team.

Conclusion

Code review is one of the most important and most effective ways to improve as a developer. In reading and commenting and critiquing code, we not only provide our fellow developers with valuable feedback that can help them consider things they may not have, but their comments can help us to improve our code as well. I find it very valuable for developers at all levels to review code produced by developers of all levels. This includes having junior developers review code written by the more senior developers. The reviews may be more questions about why things were done a certain way which can lead to more and better understanding of the code by the junior developer. For reviews from senior developers on code written by more junior developers, the questions can lead to questioning assumptions and making them aware of other ways of building the software they may not have been aware of. If you’re not already practicing code review on your team, I cannot recommend getting it started highly enough. See you next time.