26 July 2016

20 Tips for an Effective Code Review



It is a well-established fact that most of the bugs in the Software Development life cycle could be prevented literally right at the source (code). Since Code Review is almost an inevitable process in the Agile paradigm, keep in mind these 20 tips/guidelines (in no particular order) to become an effective reviewer of code. This is not restrictive to any one language but applicable to all. I've been reviewing code for many years and one of my core successes lie in stressing these points across the team. This is also the only way to effectively nurture and scale teams across the organisation.

  1. Identify the right tool: Identifying the right tool is very important. Because one should not be thrown off for adding a review just because the tool is not efficient enough. There are many open source tools out there. In most cases, you may have to host it yourself or you can also opt for services that do the hosting for you. If you are an Open Source Contributor, you would know how effective Github can be which also happens to be my personal favorite.
  2. Pre-conditions/Checklist: Any patch or a pull request that is submitted should have a minimum set of pre-conditions like it should have a Green build. A lot of Review tools have hooks to be configured to poll the SCM automatically and run the build. Build tools like Jenkins, Travis support these with minimal to no configuration. Ensure that you use them! Because it will definitely save you time and heartache instead of seeing stuff getting pushed to your trunk/master/production branch.
  3. Avoid Repeat Mistakes: As Gustavo Fring from The Breaking Bad rightly says "Never Repeat the same mistake twice", it is crucial that repetitive patterns are broken. In the context of code review, this means the developers should not get the same review comment that they had received earlier. This ensures that with each Iteration - the Quality of the patches improves so that if at all any new review comments are there - they are only new and anything that is given in the past are assumed to have been implemented in the later ones. If this is not happening, it is up to the Reviewer to go and identify to see where the leak is.
  4. Self Review: The person who is submitting the diff should first review it him/herself. Many obvious things like debugger statements, extra/missing files, ignorable files could be identified here. And I would recommend even doing a full fledged review of his/her own code as if he/she would do another one's. This culture also reduces the burden on the reviewer of concentrating on the Meat of the patch and not the obvious ones.
  5. Design Review: The Reviewer should also be able to decode the design introduction/changes that the Pull Request has and should be in a position to judge and give appropriate feedback. This is very important. 
  6. UI Review: Although software developers look at just the code and give feedback (because that is all they can be seeing in a Pull Request or a diff), they often neglect how the end product would look in a browser or the device where the code was intended to. It is extremely difficult to guess on how it will look. I recommend everyone to go the extra mile of looking how it looks and whether it relates to the original functionality. This is going to take some extra time. In my experience, this has insane returns in terms of identity and squashing obvious UI related issues. 
  7. Non-Logical Checklist: Code Review does not only involve in vetting the Logical Integrity but also some non-logical things like Naming convention, Spacing/Indentation, Object oriented compliance checks. Ensure that there is such a checklist in the first place.
  8. Keeping a pulse on the industry: This is true not only in this context but also on the overall wholeness of a Programmer. You should be up to date on what is going on in the Programming world, at least in the particular language you are part of. Knowledge of things like critical security patches, feature additions, language enhancements, performance improvements proves to be really powerful in assisting an effective review process.
  9. Encourage feedback: One does not always have to agree with what is said in the Review. If there are some contradictions - it is best they are addressed between the Reviewer and the Reviewed (or Reviewee). I also encourage that all the review comments are responded to. This process gives confidence to the entire team that any review comment will not go unanswered. 
  10. Avoid Oral Reviews: When a patch or pull requested gets created that too for teams and developers co-located or sitting next to each other, it is tempting to just go through it and give all the feedback orally. This may be fine if the team is small (only 2) and they fully own the codebase. However, this has some negative effects in terms of follow up and broadcasting. What I mean by broadcast is that there could be a review comment which could be applicable for the entire team.
  11. Learn from other reviews: Encourage to team members to not just read your own reviews and apply however to read the other reviews within the team. I've heard this famous quote - 'An intelligent person learns from their own mistakes, but a genius learns from the mistakes of others'. Let's make everyone in the team a Genius! 
  12. Dual Reviews: Similar to a Doubly refined sugar or Oil, the throughput and Quality of the code review could improve if it has a Second reviewer if that's possible.
  13. Review the Reviewer: It is a bit over-zealous to expect anyone coming new to the team who is relatively younger to the software development or who has not involved in Review process in the past to quickly catch up to all the nuances in the code review process. It would be nice if these guidelines are slowly implemented and mentoring/onboarding is in the organisation's culture. In simple terms, there could be a reviewer who can review whether reviewer complies to all the best practices out there.
  14. Over Engineering: At times it is tempting for Reviewers to comment on things that may look like Over Engineering work. These cases it is okay to voice your opinion to the reviewer.
  15. Enterprise Adherence: An Enterprise will have an adherence in different horizontals in terms on what tools they need to use, what style guide they need to follow, what frameworks has been used across different projects. It is up to the Enterprise Architect or the Senior Member of the team to proactively absorb all these facts and ensure that the entire review process is in Adherence with the overall Enterprise. This is crucial because each Atomic commit may slowly introduce things that could stray away from what the Enterprise would want. It may not look like a problem at all in the initial phase. However, should there be a consolidation happen across various projects - having multiple stacked apps across the enterprise would result in painful Refactors and often ends up leaving a huge amount of technical debt behind.
  16. Dependency Injection: Be wary of addition or removal of a new Library to the code. This again falls under the adherence of standards across the projects. Make sure that any introduction of a new library is well evaluated across the team and that it has enough support both in the near and long run. I have seen a lot of libraries which were started by individual contributors go unmaintained for years. Ensure that there is a strong community following and is very active.
  17. Against the Right branch: This may seem like something that may not belong here but in my personal experience I've faced this issue multiple times where a Reviewed creates a pull request against a different (or default) branch instead of the one that it actually has to go.
  18. Tech Debt Identification: During the course of the review, the reviewer may stumble upon an issue which involves a good amount of effort. In such cases, it is not advised to block it and hamper the delivery commitments. Instead, the right thing to do here is to add these things to a technical debt backlog where it could be groomed and picked up in future.
  19. Copy Paste excuses: "I did not do this - it was already there - I just copied/moved it" - Yes this is a very common statement every developer says when his code is challenged - however ensure that any code that is touched has to comply to the coding standards set by the team.
  20. Make Guidelines explicit: It is a very good process for all the developers on-boarding to a new team to have a set of guidelines (you could use this) explicit and review it from time to time. This could be done across the organisation.

The above list may look overwhelming. However, if you have the knack and right drive to implement some or all of these - the productivity of the Engineering team would increase by multi-folds.

Cheers!
Braga