Before I dwell into practices that I find useful for improving the code readability, I would like to describe one of my favorite tools for keeping the code clean – code reviews. While it may seem cumbersome and overhead to some, having someone else to have a look at your master piece, you will be surprised how many different things one can find in it. The reviewer might point out to some things that you might’ve overlooked or forgotten, or even point out to a better way to implement a feature, or a piece of it. For example, in the simplest case, a reviewer could have a better knowledge of the source code of the system, or part of it. During the review, he can point it out for you that a method that does exactly the same thing already exists, so that you can remove it. Even better, you could analyze both methods, choose a better implementation, and remove the other one. You know the saying: “Less code, less bugs.“?
In my opinion, everyone should participate in code reviews – from the very beginner, junior developer that has joined the project yesterday, to the senior developer, who has a vast knowledge of the system, and many years of software development in his fingers. The reason for believing so is that I think that everybody has something to offer. It may be expressed even in a form of a simple question when something is not clear, and yet the benefits can be huge. It will trigger you to think about it one more time, and maybe figure out a better solution.
I believe that code reviews are a very good way for one to learn. By performing them, new team members can get into the business domain faster and also see how different technologies/frameworks are being used on the project. They may see things that are unclear to them and immediately ask for clarification. As for the senior members, they can spread the knowledge and experience by doing the review to the less experienced colleagues, and are making sure that the things are going in the right direction, technology wise.
There should be the same ruleset for everybody regarding commits to the source control, and it should contain a rule that no code is pushed to the master development branch, unless at least one developer did a code review for the patch and approved it. Using a source control tool like git makes it much easier to perform code reviews before the new code is merged with the rest of existing application.
Even if you work on your own project, and you are a single developer, you should review your changes before you commit the code and mark the task as done. You’ll be surprised how many things you can introduce during coding and find in the review, even for your own code. And you never know, this same project might become your showcase project when you apply for a job position. Believe me, when you do that, you want to have a nice looking code for somebody else to see. It is easy to continually care about the quality of the code, and have it as good as possible. When applying for a new position, when you want to have time to prepare yourself for an interview, and stress and time constraints, it’s much more difficult to clean it up in bulk, so you’ll appreciate to have it ready.
I personally have a habit of doing a code review to myself, before I ask somebody else to do it. It’s a great way to prevent being embarrassed for forgetting to fix some simple issues that I might have skipped because I was focused on other things.
There are various techniques for the code reviews. I will not go into each of those, but will only explain how I do it and the positive and negative sides of each of them.
Before I explain the techniques I use, I’d like to share also some of my thoughts, how to approach the code review.
One of things I’ve noticed is that there are always one or two developers that are eager to do code reviews, and then everybody else relies on them to do it. If they are to busy, or absent, the others are reluctant to do it. That is one of the reasons why I ask of everyone, on my projects, to participate the review process. No matter the seniority level, all team members should do it. As I already mentioned, I strongly believe it is a great way to keep the code as clean and readable as possible and it is a great tool for introducing new team members to the project.
If you are a reviewer, don’t take the review lightly. It may make more harm than benefit if you superficially do it, and approve it for being merged, and only later on people find out that it’s a big ball of mud, and not a nice looking piece of code. Take the time you need to prepare yourself. Ask the author of the patch the push the branch to remote, so that you can have a look at it yourself for some time, before you go through the code together. This will let you have a decent image of what is made before the review, and you could already some questions in mind, after the first look.
Try to have a look at the bigger picture. How the changes fit in with the existing code base? Are there any duplicate, or similar methods introduced, that could be avoided with better code organization? Are desired architectural decisions applied to the changes? You could make yourself a checklist of things to have a look at the code at, and go through it each time you’re a reviewer. Even if you’re an author of the code, you could use the same checklist to make sure that your code with pass the review with flying colors.
Peer to peer review requires that both the author of the code and the reviewer are present. It is a synchronous process where one shows changes made in the patch and other one is trying to find and pinpoint the potential issues or things that could have been done better.
Author of the code can use the diff tool to show the changes in the patch to the reviewer. Assuming that you use git, you have a tool name git diff coming with it. It is very powerful, and can help you do the code review. All modern IDEs have Git integration, so it’s very easy to see the changes, add comments and implement a fix immediately. After all comments are resolved, you can make a commit and merge your branch to the development branch.
I will not go into the details on how to use Git here, however I strongly suggest that you have a look at the various workflows that it supports. You can find a very nice article with explanations here. My suggestion is to skip the Centralized workflow, which mimics the Subversion, and have a look at the Feature branch workflow or Gitflow workflow.
On the downside, the peer-to-peer review requires much more time than the tool supported, asynchronous review. Sometimes, when the change is bigger, or more complex, it can take more than an hour, from my experience, which can add up for several hours in a day, if one participates in several reviews. In order to get the most benefit from it, the reviewer should be able to prepare himself and get some knowledge about the implemented feature, the affected part of the system, and architecture and technology that are used.
Tool supported code review allows the author to create the so called pull request, to merge his patch to the main development branch. It does not require simultaneous presence of both the author and the reviewer, which makes it very convenient for distributed teams.
Again, in the process explanation, I will rely on git, and in this case GitHub, which gives you a nice, graphical overview, of all the changes that have been made in a pull request.
The reviewer can go through all the changes and think without interruption about each of them. He can pay attention to details, which can be easily skipped if peer-to-peer review preparation is done poorly. GitHub handles comments nicely, so each comment on a pull request is delivered in an e-mail, and even the team members that are not directly involved in the review can see what the comments are, and learn from them. Also, it’s very easy for everybody to participate in the review, and even if they don’t make the comments on the code, they can see what the changes are, which comments were made on the pull request and how it’s all resolved. I always ask all of my team members to participate the reviewing process because, as I already mentioned, I think it’s a great way to learn and share knowledge.
Please share your comments on how and if you do the code reviews regularly.