It is often said that Code Reviews are by far the most effective way to identify bugs in software. However, it is easy to notice that the "code review" term is a bit overloaded and it might mean different things to different people. At Bright Inventions we are still learning how to take the most out of this practice (like everyone in our industry, right?) and we have already gone through a few stages of our approach to code review. Let me share a few thoughts about it.
Is Code Review about semicolons?
The simplest association one may have with Code Review is that it is all about the consistent code style – to ensure the code is formatted well, all spaces are replaced with tabs or all tabs are replaced with spaces, there are semicolons everywhere or there are no semicolons anywhere, etc. While this is the simplest kind of thing that reviewers might look at, these things are too easy to waste our time on it and it should be handled by automated tools like shared IDE formatting settings (available for example in JetBrains IDEs) or static code analyzers like ESLint or TSLint plugged into our Continuous Integration environment.
There are obviously cases that automated tools are unable to identify so we should look for these within Code Reviews, but having a unified and consistent code style is definitely not the only way we can benefit from doing Code Reviews. We should definitely look deeper.
Code Review is an ideal chance to:
- share language- or platform-level idioms & practices – each technology comes with its own biases and conditions it works best within; let's ensure we're using our tools correctly, fully benefit from its features and avoid its traps,
- share general programming patterns – there is a plethora of design patterns, it's hard to know all of them and none of them is a universal silver bullet, so it might be worth looking for these that should be used but are not and vice versa,
- ensure readability and maintainability – if the reviewer can't read the code as prose then there's a room for improvement,
- discuss naming and abstractions – let's ensure the team agrees on what are these things we have in our codebase and that they actually and correctly represent the ideas and concepts from the domain world,
- consider the impact of the proposed changes for backward compatibility, performance, accessibility, stability, resource consumption and dozens of other aspects of running software.
It's also about longevity
All of these aspects are contributing to the overall project quality and it's obvious that the projects that present generally higher quality are easier (and cheaper) to maintain. But there is more.
One of the problem we tend to struggle is about the code ownership. When each team member works on the clearly isolated tasks, there is a very little intersection between code written by different people and some code ownership silos are created. It means that it's hard for another person to take over the task in case of that necessity. And that kind of necessities happen all the time – people might get sick, they may go on vacations or they just move on to other tasks, projects or jobs, so no ownership silos should happen in a healthy project.
How is Code Review related with this problem? By continuous Code Review happening as an integral part of the development process, we're ensuring that there is no line of code that hasn't been seen by another team member. Code Review might be an enabler for the collective code ownership. It may support code reuse and help the team establish a general shared knowledge about the codebase. The team is then also able to identify cases such as overlapping, conflicting or missing features or just to ensure everyone on the board understands the problem domain and the tasks on hand the same way. And this leads us directly to the next thought:
Maybe it's not only about the code?
What if we take the idea even further and put much more emphasis on the "Review" part of the Code Review than on the "Code" part? Given that we already spend time talking together and looking at what we've done, maybe it also makes sense to think if the piece of code we've just written is the thing we should have done in the first place. Maybe for the sake of feature consistency it should be written differently? Maybe it is not needed at all as the task might be already accomplished somewhere else in the product we're building? Maybe we have missed an important requirements update? Or maybe the requirements were not clear enough and we've just assumed too much based on it so we need to take a step back and figure out what are the goals and expectations for the given task first?
Taking this idea even further, let me throw the question – how much of the work that is normally attributed to the QA part of the software development process (and possibly to the dedicated SQA people) can be done within the (Code) Review? How many bugs and deficiencies can be identified earlier? And – as a result – how regular Code Reviews influence the development cost? Can it actually make it lower despite the time we need to invest in it? My unquantified gut feeling is that it's worth it.
What should Code Review look like?
The best summary of what the Code Review might look like, which is in line with my wide understanding of this process, I've found in SmartBear blog article. Let me quote it here:
Does my code compile without errors and run without exceptions in happy path conditions?
Have I checked this code to see if it triggers compiler or static analysis warnings?
Have I covered this code with appropriate tests, and are those tests currently green?
Have I run our performance/load/smoke tests to make sure nothing I’ve introduced is a performance killer?
Have I run our suite of security tests/checks to make sure I’m not opening vulnerabilities?
Does this code read like prose?
Do the methods do what the name of the method claims that they’ll do? Same for classes?
Can I get an understanding of the desired behavior just by doing quick scans through unit and acceptance tests?
Does the understanding of the desired behavior match the requirements/stories for this work?
Is this code introducing any new dependencies between classes/components/modules and, if so, is it necessary to do that?
Is this code idiomatic, taking full advantage of the language, frameworks, and tools that we use?
Is anything here a re-implementation of existing functionality the developer may not be aware of?
What is also inevitable while trying to make Code Review an integral part of our process is that we should expect every team member to participate. And this participation should have an equal status – Code Review is not about the more senior developer serving as a gatekeeper for a junior's code. It's also about juniors reviewing senior's code with the same sense of seriousness. And if a junior is intimidated when asked to comment on the senior's code – just think: is there any better way to make a progress than reading a good code? And seniors are not unerring – when the less experienced person needs to ask questions to understand the analyzed code, maybe it's not that good after all?
Last but not least, a successfully finished Code Review should become a part of our definition of done - unless the code was reviewed and accepted by another team member(s), we can't move on and mark it as completed or treat it as finished. It should be treated on the equal rights with writing unit tests – not an optional addition we fancy from time to time, but a thing that – when missing – causes a warning flag to be raised.
Originally published at brightinventions.pl
By Adam Bar, The Web Guy @ Bright Inventions
Personal blog Twitter Email Github
Top comments (6)
A great article, glad to see some details on the subject.
One thing I've always wondered, since I've only worked at 1 company that used code reviews, is how people structure their code reviews. Can you share on how it works for you guys, in terms of who reviews who's code? Is there one senior dude that always reviews? Or is it just a regular task in the development workflow, so that anyone can just pick up the task and do it?
Well this is a topic that bothers me personally – how to create an environment that encourages every team member to participate in code review on equal status. I mentioned in the article that I find it beneficiary that everyone acts both as a reviewer and reviewee. Right now, until the process and the organization matures enough, we often proactively ask more junior guys to take part as reviewers just by assigning the review as a task for them. But I hope it will once become a habit and the part of a normal workflow like it happened with CI or unit testing for us.
In my experience, code reviews are a compromise between the extremes of no code reviews at all, and pair programming (which would be continuous code reviews).
The positive aspect of code reviews is that it gives another pair of eyes to find problems, and the early a problem is found the cheaper and easier it is to fix.
The negative aspect of code reviews is that -- unless the reviewers are intimately familiar with the code -- they tend to be a gloss level code review and find immaterial problems like "where the curly braces go" or "tabs used when spaces should be used" (or vice versa, in that perennial bike-shedding flame topic).
I tend to put code reviews in the same category as writing unit tests after-the-fact, or closing the barn door after the horse has bolted.
I think that pair programming (and hence continuous code review) works far better. Alas, I've only had the experience of pair programming a few times, as it has been in strong disfavor by management.
Similarly my experience with unit tests is that there is huge value to doing unit tests in the test-driven development style of writing the unit tests first; but they lose almost all their value if written in arrears. That value being to influence the formation of the code (the implementation level design).
Oh, there was one environment I was in where every team member was expected to do code reviews at their leisure for all check-ins, after the check-in had already gone into the source control repository. Completely informal, rather than formal code reviews. That worked pretty well, with higher quality reviews, and had less scheduling bother than trying to coordinate code reviews before check-in. It works because, as it turns out, code is extremely malleable and can be easily changed.
Thanks, it was a great article on Code Review! the eye-catching part for me was that you were using Code Review to improve the overall code ownership. Can you explain how do you define and measure ownership and how do you decide who should review the code?
Good write-up.
How often do you request code reviews? Is it for each commit or for each pull request to your main branch? Or something entirely different?
Thanks.
Our rule of thumb is that it is more or less correlated with feature branches. One feature = one review, so it might contain (and most often contains) multiple commits. However, for small features, we tend to rebase and squash it into a single commit to master after the review.