This is a guest post by Błażej Hadzik of netguru.
Peer code review is not about criticising individual mistakes. It’s about making progress on a project as a team and keeping everything in good shape.
What is code review?
Code review is a process by which developers review and evaluate each other’s code. The goal is to make a project better, faster and more secure through bulletproof code.
It’s also an opportunity for people to learn good code quickly. For newbies and mid-level developers, it’s a free and accessible way to see how and why things work the way they should. Code review often sparks discussions, where code snippets are passed around and someone always learns something new.
To sum it up, code review is not just about the process of review itself, but about learning and teaching others.
Push something to repo where others can see it and suggest better solutions or better gem usage. Learn something from code review and you’ll bring it to another project as well.
I do it every day and it works like a charm!
Why should I care?
There are a bunch of reasons for doing a code review for you as a developer, or for a client. First of all, it reduces the amount of risk. When you write specs, you can catch some major bugs that might break your app. But you cannot test everything, especially minor changes. Here is where code review comes into play. Checking even small conditions, loops or HAML changes can protect the app from bugs that can turn into larger issues.
Second of all, it allows us to add quick fixes or corrections to our code. If someone caught a wrong indent, comma or space, I would push another commit with fixes and no harm would be done.
Another cool thing is that you can get great feedback from your workmates and other developers. Having code review done is one of the easiest and fastest ways to be evaluated by others. People do a code review and evaluate each other because they know you will do the same for them, and that is how it works.
Gaining kudos is another plus. Writing good code and following best practice definitely will not be forgotten by experienced developers. Be a hero, and one will show up in your time of need.
The ‘happy medium’ rule
There is a thin line between an accurate and an inaccurate code review. I call it the ‘happy medium’. I would say that checking more that 20 commits at once or more than 200 lines does not work for anybody and is impossible to do right. It’s proven, too, so don’t even try!
Conversely, reviewing one commit or a couple of lines won’t teach you anything, nor will it give any solid feedback for the author of the commits. The code review matters only when you care, so don’t think about it as a duty, but more like a lesson or fun task.
The culture of feedback
Although most of us are smart, friendly and open for feedback, there are still some people who have an ego. It’s really important to show them that everyone makes mistakes. It’s also a good idea not to be a pain in the butt for others. As I mentioned before, code review is not about pointing out mistakes but about learning and teaching. Don’t try to show how very perceptive you are about people’s mistakes. Always try to help and figure out the best solutions for each specific case instead of offering only criticism. People love constructive criticism, not selfish developers.
The netguru flow
And now for The netguru flow. This is how code review works for us, and why we love it. Every time someone pushes something to master branch, the commit gets a ‘pending’ state assigned in our review app . This means a commit needs to be reviewed. We then have two office days to ask someone else to evaluate our code. If it’s not reviewed, the commit will auto-reject (expire).
A reviewer can write comments and click an ‘accept’, ‘reject’ or ‘pass’ button. Passing a commit means that the changes will work properly, but you suggest that it does not follow the best practice. The sql queries might be slow, or for some other reason as a reviewer, you would suggest better solution. Once the commit is passed, the author has another 48 hours to fix it.
A rejected commit means that the changes might have a bad influence on how the app is working and will probably crash the app itself. The rejected commits should be fixed immediately by the author.
The whole process is strictly connected with CircleCI, our continuous integration tool. After running specs, the CircleCI makes an API request to our review app and checks if there are rejected or auto-rejected commits. If so, the whole deployment process is stopped and changes won’t be pushed to the server. This helps us (and forces us) to have our code consistently reviewed. The result is that we can deliver the best in quality to our clients.
To sum it up, code review helps everyone involved, and it speeds up and secures the development process. Thanks to review processes and reviewers, we catch bugs at very early stages and can focus on making great progress, great products and great developers.
Photo: Markus Spiske via Flickr