Code reviews are one of the most useful parts of the development lifecycle. There are many different approaches to code reviews, and figuring out best practice for yourself and your company can be hard. You have to do what works for your scenario, but there are certain things that all of us should be doing as part of code review.
In a code review, a developer passes a finished piece of code - be that a new feature, bug fix, or change to an existing feature - to another developer to be reviewed, before it is considered ‘done’. This process has many benefits for both the developer and the company.
The main reason to perform code review is to ensure that code quality is maintained. There is never a single solution to any problem, and by having at least two developers review any piece of code, we are more likely to create the optimal solution. Having a second set of eyes will often reveal things that the original developer did not think of, but also prevents laziness and stops developers taking shortcuts in the code in order to complete a job more quickly. Bad code will cost you down the line.
Code reviews also add a QA test layer to the development process. Every developer should test the code they write, both manually and with automated tests, it is easy to miss use-cases when you are the one writing the functionality. Getting lost in what the code should do, rather than what it should not. Having another developer both read through your code and test it (manually and by running automated tests) will help confirm that mistakes were not made and that the code is . Unfortunately, manual testing is often skipped during code review, and because of its people often assume that code review means simply reading through the code. While this is useful, it does not provide a complete picture of what is happening.
The pull request feature provides the perfect interface for delivering a code review, allowing you to comment on individual lines of code, or on the entire change. Once a reviewer has completed their code review, they can then approve the pull request, request changes or just comment without either approving or rejecting.
Due to the nature of the code review process, only constructive feedback is given. While there is no reason not to provide positive feedback, there is no benefit to the code in doing so, so it is usually seen as a waste of time. Positive feedback is no feedback. As a result of this, it is not uncommon when implementing a new code review process for developers to feel disheartened or victimised by their code reviewer. To prevent this, when implementing a code review process (or hiring a new developer), all developers should be made aware of the expectation of the process. While this is not a complete solution, after you have been through the review process a few times, it becomes normal. The aim is to foster an environment where developers strive to get their code through code review without any comments at all.
When implementing the code review process, it is a good idea to start out with a checklist of things to look out for. This ensures that all your developers are performing the same checks. While it should not be necessary to check code formatting and coding standards, provided your developers are using correctly configured linting tools, it is often the first item on a code review checklist. Other items may include: manually testing functionality, automated tests passing, automated tests covering all use and error cases, no known security vulnerabilities, no obvious poor performing or unnecessary code, testing. You will need to design a checklist that works for your team and your company's requirements. Once your team has become used to delivering consistent code reviews, you will be able to phase out the checklist, although to ensure consistency, it can be a good idea to continue to use it for new developers joining your team.
Top comments (11)
Hi Collin, tthis all depends on your development process. We dont have a dedicated QA team so rely on the developers and our account team for QA. We find that developers who are familiar with the code anticipate bugs better and catch them earlier. This means that by the time the account team see any work it is generally bug free and so saves them time. I think this can be the case with a QA team also, you want to foster an environment where your developers strive to not produce any bugs at QA stage, the team should work together to ensure the work is bug free before going to QA. C.R should be seen as a way for your developers to help improve each other and the system. That is, after all, their job.
It is not uncommon to get push back from senior developers on this, but if the rest of your team are behind it and drive it. It becomes part of the process and they won't have a choice. We integrate it into our definition of done, so no piece of work goes outside the dev team until it is fully reviewed, tested and unit tested.
In my team, we see the creation of good unit tests as part of our culture. At first it was hard to get on that discipline harness. Now, in our most complex library we have over 800 UT that run within 20 seconds (93% code coverage). The feedback on any change is great. It took some time but it is worth the work. Check out Michael Feathers' book "Working Effectively with Legacy Code"
Great to hear more people writing unit tests. :)
In my team, we have a git-hook set for pre-commit where we run lint and unit tests. If either of those fail or the coverage threshold was not met, then the dev will have to fix the issues before trying again to commit. We also use a tool called jest-coverage-ratchet, which updates our coverage threshold after a successful commit.
Our team and organization goal at Build.com has been to reach 80% branch coverage. I have personally set a goal for myself as well, or more like a rule that I will write enough unit tests to meet the 80% branch coverage for any file I touch when working on a particular feature/bug. For non-complex files, I usually try to meet 100%.
Thanks for the positive feedback john. Implementing a strict CR process on small personal projects is hard but pays off in the long run. My advice is to lock your github repo master branch so you cannot push to it or merge without an approve CR on your PR. That will help you enforce goid practise.
As for reconsiling your technical differences, id suggest a few things. First off agree between you what a CR is going to include, then make a checklist. Dont go into too much detail. Some of the other comments have good suggestions for this. Ensure you are both manually and automatically testing the code, beyond that. Just do what you can. You will learn a lot from both having your code reviewed and feom reviewing others. Use it as a learning tool you both can benefit from.
Positive feedback does benefit the code and the developer. If you see a pattern or refactor that makes the code better pointing it out to the dev helps highlight it.
As you have pointed out code reviews can be really rough emotionally for devs both new and experienced and the positive comments really help negate that.
That being said great article! I like the point about running the tests as part of the review.
I certainly would not discourage highlighting good solutions to reinforce good practise, however it is fairly common for code review comments to be mostly constructive. While I would not discourage positive feedback, I was using the point to lead onto how to foster a positive environment without developing an envronment where your developers expect constant positive praise in a code review, as that is just not productive.
I agree wholeheartedly with everything said in the article. This article is especially great to prepare you when you are about to introduce a Code Review process in your team.
For my last team, I've written a small online tool to be used for the mentioned checklist. It is very easy and efficient to handle and includes a "hierarchy" of things, so that only parts of the checklist need to be filled out that are relevant to the code under review.
You can have a look at it here:
guess-what.io/c
Here is an example checklist for code reviews:
guess-what.io/c/2DglcWlYmE6ITeHAlB...
I hope that others will find it helpful and maybe use it on a daily basis, too.
Thanks daniel, this sounds really great, will definitely check it out! Thanks for the feedback!
Mike, thanks for the article. I can't agree more, more eyes == less bugs.
To show some choice to the readers, I'd like to add that Gerrit is (IMHO) much superior for the code review process than GitHub pull requests. It is also free (both as in speech and as in beer), can be hosted internally, and there are already public SaaS allowing to use it, for example GerritHub.
Surely it has its twist for a git workflow it requires, and takes some time to get used to, but it really pays off IMO.
As for another set of tips for code review, take a look here (and immediately figure out what project I'm involved in :) )
Also, one must CI all things. Manual testing is flawed because it is 'manual', and humans are fallible. Have a decent unit test suit, a functional test suit and (if being a part of a bigger picture) an integration test suit - and run them automatically at least before merging, but better on each iteration, and let the CI vote on changes too.
Thanks Pavlo, I have not heard of Gerrit before. Will check it out! I like that Github has improved their code review stuff recently, coupled with all the other fab features it just seemlessly fits into many peoples current development processes, but its great to have alternatives! Just glad we are not back in the days where we had to do it on paper or word! Thanks for the other article too!
We struggle with the same thoughts. If budget and time were no issue we probably would have one, however based on the frequency (or infrequency) of serious bugs found in our application and the speed at which we are able to fix them currently it would not be cost effective to have even a single QA tester at the moment. Although we would still have our developers test their/each others code before going to QA.