Friday, July 11, 2014

Case for Code Reviews

I have been doing code reviews for about six months at 3CInteractive.  Since I'm so new at it, it's hard for me blog about "best practices" or even do a presentation on code reviews.  Therefore, I decided to have an open space meetup at our Miami JVM Group.  The open space will be around "effective code review process".  I'm hoping to learn about the following: Who is using it?  Who are not using it and why?  Who thinks they're not useful and why?  What have they learned?  What were some of their challenges?  This post is based on my experience and some of my lessons learned while working with internal and offshore teams.

Benefits of Code Reviews

  • You write better code when you know it will be reviewed.
  • A second (or third, or fourth) set of eyes will help spot defects.  This is very similar to pair programming, but it works even better if you're working with an offshore team.  It's also a great way of learning new APIs.  For example, someone could tell me that my code can be easily done using the Guava library, or that the code is actually an "aggregator" Enterprise Implementation Pattern, and I should probably look at Camel.
  • More than one person understand your code (cross-pollination or avoid silos).  Having more than one person look at your code helps spread the knowledge and context of the problem or solution.
  • Reducing the learning curve for new developers.  I believe that even junior developers should be part of the code review process.  It's a great way for them to learn about the code base and they become more productive.

What To Look For

  • Bad design.  Highlight issues such as SQL injection, look out for lack of design patterns, or anti-patterns.  Things like separations of concern, encapsulation, and apply certain basic OOD principles - DRY, encapsulates what varies, open-close principle, etc.
  • Performance hazard.  For example, memory leaks.
  • Lack of clarity - the application should work and the code should be readable.  For example, a class named "SomethingServiceImpl" with no documentation on the class will be highlighted and will prompt a change request to the developer.  Also, a big nested if statement that is not quiet clear will prompt a change request. 
  • Not consistent or not according to standards.  Having a set of standards makes code reviews a lot simpler.  It also sets a norms for the team.  For example, not using Domain-Drive Development standards, consolidating APIs (Guava vs Jakarta Commons), having a handful of languages, and having a code style rules.

What Not To Look For

  • Premature optimization.  Don't try to optimize all of it at once.  As my buddy Tyler mentions, "make it work, then make it better".
  • Skills and expertise gaps.  In our company, we allow all developer to do code reviews, including junior developers.  These developers gain a lot of knowledge about doing code reviews.
  • Personal style.  If the CTO goes over and says, "I wouldn't do it like that" it bring noise to the process.

Quality and Dissemination of Knowledge 

My team is responsible for improving code quality, lower defects in code, improve communication about code content, and teaching and mentoring of junior developers.  Code reviews has helped us on shorter development cycles, more customer satisfaction, and more maintainable code.  But most important, it has help us spread the knowledge and norms.  As per the book 97 Things Every Software Architect Should Know,
Chances are your biggest problem isn't technical
Most projects are built by people, and those people are the foundation for success and failure.  So, it pays to think about what it takes to help make those people successful.