If you’re a programmer (or what we like to call today, a developer) you have most probably heard that implementing a code review process is good. If you have never heard of the term before, jump over to Wikipedia and have a quick glance at this relevant article. We’ll wait for you to come back.
Code review helps to eliminate a good number of coding errors or poor design decisions, before they hit production. If you’ve ever developed real life applications you’d know that bugs or questionable architectural decisions that reach production will come back to bite you (badly). Applications that have been out in the wild for long enough are already dealing with some (or more) technical debt. The least we want is to raise it.
With code review, a lot of problems can be caught early on and be fixed while the code is still fresh in the developer’s mind. Even if you have already implemented unit, functional and integration tests and you might think you’ve done enough to stop bugs, these numbers might change your mind.
Catching problems early will also take some burden off the QA team and it will allow you to stay on schedule; you’ll have to deal less with unplanned hotfixes (and angry users) midweek. It might even help pass knowledge from experienced programmers down to more novice ones and create a proper coding culture. So, if you weren’t already convinced of the benefits then you must be by now. But the real questions come when you try to figure out how to implement a code review process. And that’s one more example in computer science with no silver bullet answer.
Each team has their own way of doing things and code review should adapt to that and to what is at stake (the cost of failure). If you’re building space rockets you’d want to invest in a very thorough code review process. Imagine spending several billion dollars over 10 years to lose a rocket half a minute after takeoff because you tried to fit a 64bit number in a 16bit variable (a not at all fictional example). In most cases a full blown formal code review with two weeks of planning and five meetings is a definite overkill. In a start-up or agile environment there’s practically no budget or time for this. Surprisingly, research says that formal code reviews are not that much more efficient than informal ones!
So let’s look at how do we do things here at codebender. First you should know we use github for our code repository needs. Github already has some collaborating features that we adopted to accommodate our code review procedure. A developer starts working on a feature by creating a local branch with the feature name (or something close) and commits code. At some point, pushes to the repo and creates a pull request (from now on referred to as ‘PR’ to merge that branch to the development branch. At this point, work is usually not finished, so the PR is marked with a label as WIP (Work In Progress). We want people to push early so everyone else has an idea of what is happening and maybe even look at the code and offer comments.
When work is finished, the WIP label is removed and a Ready To Review label is added. At this point the developer can continue to other tasks but the previous task is not considered finished yet. The reviewer will visit the PR in the same or next day or so and if all is good, mark it as Reviewed and Ready for QA. The QA team will pick up from there and if all is good the PR will see the light of day (be deployed in a production server) in the upcoming week. A ’failed’ code review or QA test will return the PR to the original developer and then the process will repeat.
We have a clear rule to follow: “Thou shalt not push code to production unless it is reviewed by a person other than the original coder and automated tests and QA has passed” (read in deep voice with thunders striking in the background). This might seem dogmatic, potentially slowing us down, but we still adhere to it and nobody challenges it because we’ve seen the benefits.
What are we looking for in a code review? The reviewer must first understand what the PR is all about. What problem is being addressed? What was taken under consideration to reach the suggested implementation? The PR description should explain all this. Then it’s time to dive into code. If the PR is really small (not many lines or files changed over a couple of commits) the comparative split overview is really helpful. Otherwise we need to go commit by commit. In both cases we’re looking into the implementation, trying to figure out what is happening, why, if it is what is intended, what problems it might bring on, if it utilizes resources properly or if there’s a better way of doing it. We use inline code comments to discuss code. Github is really helpful with that.
In most cases a reviewer doesn’t need to ’interfere’. Even if nothing is as the reviewer would code, code review is not about the idea of the exact implementation the reviewer has in mind. If the given solution is sufficient and won’t cause any problems, jumping in to offer an alternative is counter-productive; it offers no added value and opens space for long, meaningless discussions that revolve around egos. An exception could be made for when the reviewer might want to offer an alternative elegant solution, but that is rarely a reason to turn a PR down. Hopefully it will be taken into account by the developer the next time.
Equally, when some code snippet is challenged during the review, the developer shouldn’t be taking it as a failure in a competence evaluation. Code review in not a personality review. It’s only about the code. A developer should take note of the comments received and make sure to take them into account. There’s nothing worse than getting the same comments over and over because it shows no improvement. It is the developer’s job to make sure code, commit messages and PR descriptions are polished and shiny and follow conventions and coding style. That’s not only for the reviewer, but also for future reference if someone needs to revisit this code. Comments inside code should be verbose when something unexpected or unconventional is happening but, the rest of the code should be clean and readable enough to not need comments.
It is of high importance for the team to realise the reasons behind code review. Neutral comments and approach will help point out it’s not about personality but about code. When that happens all is done without bad feelings and the code base grows without letting monsters lurk inside.
Our process works for us but that doesn’t mean it will work for everybody. Your needs might be different and even the size of your team might influence the process. If you’re attributing in open source projects, your PRs are evaluated by peers before merging. Nobody calls that code review, but essentially that’s what it is. Even if you’re a stand alone programmer, don’t be afraid to try the codereview community of stack overflow, either by asking for feedback or responding to one.