Recently we’ve had some discussion on the PHPNL Slack community about code reviews. It’s an interesting topic that I’ve been meaning to blog about for quite a while already (and I’ve written about it for Wisdom of the elePHPant), so this was a good reason to sit down and actually put down my thoughts on code reviewing.

So what is it?

Code reviewing is exactly what it sounds like: It is reviewing code written by another developer. There are different ways of doing this, but in the end it all comes down to having at least one other set of eyes checking any code written before it is released.

There are many reasons for doing code reviews. It can be to prevent security issues, to ensure correct performance of your application, to prevent bugs but eventually it all comes down to the more generic term of ensuring the quality of your application.

How do you do code reviewing?

There are many tools that will help you with code reviewing, and I can’t give you The Perfect Tool For The Job. What currently works well for many people is using the approach with Git where you send a pull request. Most Git-tools (such as Github and Stash) have excellent interfaces for going through the diff between a feature/bugfix branch and the main branch. They allow you to add comments with every line of code, so that the author of the code knows exactly what you’re commenting on and is able to fix the right code easily. Similar tools also exist for many of the other version control systems out there.

While tools can make your job easier, it is not necessary to use them. You can go through a diff when using your version control system from the commandline, or you can even go through the code together with the author of the code while sitting next to eachother (or while using screensharing with Skype/Hangout).

Preparing the review

As a developer I always make every change (feature, bugfix, improvement) in a seperate Git branch. The branch is pushed to my fork of the main repository, so once I’m done with my change I push the branch to my fork. I then create a pull request against the main repository. The PR description should contain useful information: A link to the (Jira) issue this is a PR for and possibly some generic information on what you’ve done in this specific PR and what choices or assumptions were made.

When the PR is created, I immediately review it myself. Reading a diff on Github or Stash is completely different from working in your IDE, is my experience. I look at my code in a different way and often I spot small bugs, forgotten changes or useless changes in this mode that I don’t when actually working on the code. I often also spot forgotten docblocks and such pretty quickly when reviewing my own code.

Doing the review

When I sit down to do a code review of code written by someone else I open up the pull request I’m about to review. I click the link to the Jira issue to check what the code is supposed to do and then start looking at the code. I focus on many things: Does this logic indeed implement the feature requested, but also: Does it follow the standards. I am known for nitpicking over docblocks, typehints and such.

Depending on the testing strategy in your organization, this is also the time to check if all code has accompanying (unit) tests, and whether they are actually good tests. Testing is a subject on its own of course, but it should be part of your review to check if the tests are there and good.

For everything I find, I place a comment. Even when there are several places in a file or PR that have the same “issue”, I place a comment in each place. I’ve gotten some feedback that some people don’t like this and prefer “check this PR for x, there are several places where this occurs”. This is a personal preference, but I prefer getting the comment in every place where it’s wrong, because when running through the feedback for a pull request it’s easier to know exactly what you should update where.

When commenting, pay attention to the way you write your comment. Code reviews are a check of the code, not the developer. Comments for that reason are not an attack on the person, but should be directed at the code. Explain why something is, according to you, not the right implementation, why you’re commenting, why you think it should change. Always keep in mind that there is a human being on the other side of the pull request that needs to handle the comments.

When code and/or documentation isn’t clear: Ask questions! You can ask the developer directly but you can also add a comment with the question. Reviewing is not something you do once per pull request and then forget about, a pull request can actually contain whole discussions and conversations. It is wise though to keep this to a minimum and when it turns out there are discussions/conversations going on, try doing this directly and put the conclusion in a comment so that the pull request does contain the information that is important to decide whether the pull request can be merged.

Functional review

While working at Enrise for a project some time ago, I got introduced to the concept of also doing a functional review of a pull request. Every pull request should be reviewed by two developers other than the author, and one of the developers is also supposed to do a functional test of the pull request. This means you don’t just check the quality of the code, but also the functionality. I really like this approach. Code is only code, it doesn’t turn into functionality until you actually execute it.

Processing the comments

Back to looking at the review from the developers perspective: Once your pull request is reviewed, it is up to you to process the comments. This definitely doesn’t mean you actually have to do exactly what is said in the comments: Look at every comment and see if you agree. If you agree, implement a change to process the comment. Otherwise, respond to it to state your position on the subject. As mentioned above, when it looks like it is going to turn into a discussion, it is better to not do that in the pull request but instead do a direct conversation and only document the conclusion of that. When you decide not to follow up on a comment, make sure to explain why.

As mentioned before, a code review is not a “check once then it is fine” thing. Instead, it is a play between all participants in the code review that goes on until the pull request is approved. So when you’ve made your changes to your pull request, you send it back for review and another review may happen.

Approval

Depending on the organization you work for and its strategy in these things, once the reviewer(s) approve of the pull request it may be merged immediately or will be merged during the preparations of a release. Approval is usually the moment where you’re done, your (Jira) issue can be moved to the next step in the process and you’re done. Congratulations, you just contributed more good code to the project!

Who reviews?

I personally think that in a team of multiple developers, everyone reviews. Not just the seniors should review the code. Depending on team size, you’ll make a decision on how many people should review a specific pull request though. When you have a small team, you’ll probably want one other developer to review, but when you have a bigger team it’s a good idea to have at least two people review each pull request. This should give a more balanced review than just a single developer.

Seniority should play no role on deciding who reviews. Senior developers usually have a pretty strong opinion on how things should be done, but are sometimes also stuck in certain patterns. It can be refreshing to have a junior or medior do code reviews because they may comment on things that seniors don’t pay attention to or that they don’t see. Aside from that, code reviewing can be a great way for a junior or medior developer to see how other developers approach certain problems and implement certain features. The learning goes both ways!

Group reviews

Aside from the system where each pull request is reviewed before it is merged, it can also be a good idea to do group reviews every once in a while. You’ll need a laptop, a beamer and your codebase. One person controls the laptop and as a group you go through your codebase. Everyone is allowed to speak up when they see something weird in the codebase, which then causes a discussion (well, usually 😉 ) that can result in an improvement ticket in your issue tracking system.

In group reviews it is just as important to realize that comments and discussions are not critizing the author of the code. It is an effort to improve the quality of the codebase. As such, don’t offend and do not take offense from anything discussed during a group review.

Code reviews when alone

Doing code reviews when you’re a freelancer working on your own can be hard, but not impossible. There are many ways of doing this. The easiest is to find a group of likeminded freelancers that are willing to exchange favors. Those favors can include doing code reviews for each others projects. The feedback that another developer can give you is invaluable and can really lift your codebase to a higher level. Especially when working alone, this can be very important for your personal development and for your code quality.

Code reviews in the QA ecosystem

It is important to realize that code reviews are part of the QA ecosystem. As such, they can not replace other parts of that same ecosystem. You can not replace code reviews by a setup with PHPMD, unit tests, PHPCPD etcetera as you can not replace those tools just by code reviews. They should be done side by side to get the best out of all those tools.