Code Review in a Startup
Here I bring to you the 5th blog in the DevOps series showcasing our learning while #scalingup. Read our previous blog to know about the bunch of tactics that we used at different times during our evolution to achieve a successful clockwork during our DevOps journey.
Proper optimized code review is something that many startups miss. Some take the easy way out and ignore it and others spend years discussing the best practices, conventions and styles without ever committing code. Both these are slightly over exaggerated examples of paths to failure, but I am sure you can relate to these if you have ever tried to answer the all important question about code review “Exactly how much code review should your team do?”
For perfectionists, the answer is that code should always be reviewed and you should always be refactoring code and improving it wherever you see a scope. I have worked in companies, where people used to spend almost as many hours reviewing code as much as they would writing them. There are some practices like pair programming, which has maximizing code review as one of the results. So this is not exactly a wrong direction of thinking. The problem however is that when you are working with time constrained environments like that of a startup, you will not have a luxury where you can keep revisiting and improving code beyond a reasonable amount of time. And I have seen that surprisingly many people altogether avoid code review because timelines are constrained. I don’t even need to mention how risky this is and how dangerous this course of action is. However it seems many people do exactly that.
In this blog post, I will attempt to throw light on our experiences in deciding how much is enough. We too, like other startups have a time constrained environment, and a few (thankfully only a few) customers who want every feature done yesterday. So as a culture, we too try to make sure that we finish everything faster. Development faster, Testing Faster, Deployment to various environments faster. But we do not ignore code review. We ensure that we do code review. We have a few principles that we followed to make sure that code review happens all the time, and is neither too less nor too much. Here are they. What we have seen is that if you follow all these steps, then you have a sane code review process and you can guarantee a stable flow of god code into your repository. These points are in no particular order of importance.
You should do code review: The first principle is not a lame attempt at a joke. The idea is that code review should happen come what may. Without this principle being followed, every other principle in this list breaks down. We use git-flow as one of our developer methodologies with git. One of the advantages of this system is that code review is built in. Unless code is approved by a designated reviewer, the code does not go to the next level be it Testing or Production. The Approver is as responsible for a piece of code as the original developer. This way the reviewer spends more time reviewing and the original coder also reviews and corrects his code in advance to preempt the reviewer. This adds more points in the system where code review can happen and makes sure that code always gets reviewed and the review is not forgotten.
Requirement Matching: Does your code do what the requirements ask it to do? Are we doing less? Are we doing more? A quick inspection reveals mismatches if any. This goes a long way in finding out if there are any problems in the code.
Readability: There is an apocryphal statement that says that code is written once and read hundreds of times. While the statement may not be accurate, you may write more than once, and may not read hundreds of times, you still do get the picture. The basic premise is that code does get read many far more times than it gets written. Also once written, your code will also be read for review, bug fixes, enhancements, etc and not always by you. Also in the IT Sector, jobs switches happen a lot, so a new person should be able to understand and work with the code as soon as possible.. So it makes sense that whoever looks at your code after you have left is able to understand your code well and can alter if needed and maintain the code well till the product lifetime completes.
Reviewability: A further subset of this is that the code should also be reviewable, you and your code should be able to convey to the reviewer what the code is supposed to do and what the reviewer is supposed to review.
Scalability: Will your code be able to stand frequent and/or continuous requirement changes in the future? Will it be able to handle a reasonable amount of requirement change without having to have to rewrite the entire thing? Overall applications are live, especially in the Agile era, your requirements are never frozen, and hence the code also should never be frozen. It should be able to handle changes in requirements. A word of caution here, do not overdo this. While your code should be able to handle requirement changes, you cannot (and should not) make your code so generic that it can handle the proverbial ‘everything under the sun’. Your code should be reasonably scalable. Too much scalability also is as bad as too little. How much to go down this path will depend on your specific business needs. However it is not a bad idea to talk about specifics to your business stakeholders. They can tell you how and more importantly how much a feature will be used. You can then decide how scalable you want the code for that feature to be. The process of arriving at how much is just right, takes time to set, but once done, you will thank yourselves for the foreseeable future.
Improvements: This is one of the basic purposes of code review. This answers the question, “Can we do the same thing in a better way?”. Better way could mean one or more among, faster performance, better readability, more modularity, and others. You need to keep asking this question in a code review. If you can, then your code review is not complete, if you cannot improve any longer, then probably the code reached here after many rounds of reviews. Or was copied from well reviewed code. Again this is one of those things that has the potential to be overdone, so think carefully how far do you want to go down this rabbit hole without losing your wits.
BNBR: This is lifted from one of the policies of Quora, It means Be Nice, Be Right. The point is that while reviewing , you need to be nice and be right. Being Nice First. The point of a code review is to see if things can be done better by putting multiple heads instead of one. It is not to hurt or massage egos. What can be done by just pointing out issues with data should not descend into a shouting match (verbally or through the keyboard). Make sure that your comments are politely worded and are correct.
Code Scanners: Before you give your code for review, your code should be scanned by tools to make sure that basic checks are done for issues like parenthesis matching, formatting, typos, indentation, naming convention etc. Your reviewers will have a tougher time navigating your code if you do not fix these. If your reviewer finds these issues and not a code scanner, then you have not prepared for your review well.
The Unhappy Path: The code works fine, but some scenarios were not tested. How do you know if your code is able to handle most of the basic errors or exceptions. Your review should make sure that this is in place well before time. Again you need to use this judiciously. You should not overdo it.
Timeliness: Your code review should have a deadline. You cannot indefinitely keep reviewing the code, your review should finish within a deadline. If you ship years late, how will better code help you ?
Dark Spots: Every reviewer may not be able to review all aspects of the code. So it is a good practice to tell in the review comments what aspects were reviewed and what were not, so that everyone knows the extent of the code review. If everyone says it looks good to me, but everyone only happened to review the naming conventions, then it probably was better if only one person reviewed. If each reviewer mentioned this small info, then everyone knows in advance if there were any dark spots in that particular review and they will be able to redress it.
Fatigue: Do not review too many pieces of code at a time. If you happen to be spending a long time reviewing, then probably the code under review is too large or you are reviewing too many PRs at the same time. Reviewing is a thought intensive process and you need to make sure that it is done properly. So please take breaks, if you are tired of reviews and are still somehow powering through that will reflect in the quality of the reviews. A rule of thumb is to not review more than 60 minutes at a time or around 400 lines of code at a time.
Checklists: One good shortcut is to use checklist to review a PR or a piece of code against. These checklists ensure that your mind does not wander, wondering what you have missed and you will also be reviewing against pre decided metrics.
Defects: What do you do with the issues you found ? Not every issue needs to be or can be fixed immediately. You have to decide what to do with each review comment. Whether you will be fixing them, ignoring them or putting back into your backlog. Make sure this is a separate backlog for technical debt.
Overall these are the things that we follow while doing code review. Many of these helped us a lot to make sure that we are reviewing just enough to keep our process wrooming along while at the same time, not ignoring major issues.
