Agile Code Review guidelines

“Really thorough code reviews, in which someone else reads your code carefully to find problems, are the best
way I know to do early bug detection. But they are rarely done. This is partly because they take a lot of time
to do well, but I think it is mainly because most programmers are a bit afraid of having someone else review
their source. We as a profession need to get over this reluctance; if you don’t want someone to read your
code, then write it so you do. If someone else shouldn’t read it, why should someone else use it?”

Jim Waldo – Java – the Good Parts

I strongly believe code review is important part of application lifecycle management because it help to deliver quality code and as a result, quality product.
In traditional setting code review done formally, usually by team leader or senior developer before it committed. In agile environment, code review is more organic via team collaboration. The goal and expectation of the code should be clearly defined in coding guidelines. Goal is cooperation, not fault finding. Everybody should be on board that code review is to benefit team as a whole and every programmer in particular.

Benefits of code review:

  • Two head is better than one in catching bugs and problem
  • Keep code quality and readability high
  • Establish trust
  • Mentor junior developers

Coding standard apply independently of the language you code with. For Java development, I would be looking into following areas when doing code review:

Java code review standard:

  • Proper variable declaration: e.g. instance versus static variables, constants etc
  • Performance issues: e.g. Use ArrayList, HashMap etc instead of Vector, Hashtable when there is
    no thread-safety issue
  • Memory issues: e.g. Improper instantiation of objects instead of object reuse and object pooling, not closing valuable resource in a finally block etc.
  • Data access issue – getting to much data from DB or issuing too many calls to the database
  • Thread-safety issues: e.g. Java API classes like SimpleDateFormat, Calendar, DecimalFormat etc are not thread safe, declaring variables in JSP is not thread safe, storing state information in Struts action class or multi-threaded servlet is not thread safe
  • Error handling – throwing exception without keeping original (hopefully Java 7 fix this), not logging it into log system
  • System.out is used instead of log4j
  • Design issues: No re-use of code, no clear separation of responsibility. for example, have business logic inside servlet instead of using business object layer, visual elements (HTML, CSS) embedded on back end e.t.c
  • Documentation of code: e.g. No comments, no header files etc
  • following best practices for given framework – for example in Spring 3 use annotation instead of xml files for IoC, use external properties instead of hardcoded values for easy deployment, e.t.c

You should create a code review document and templates for the whole team and update it as project proceed and you learn more about software you choose for the project.
It is convenient to use tools to make this process easy.
Code review tools:

  1. Crucible is Atlassian’s tool for reviewing work as a continuous process. Crucible enables code review and is highly integrated with JIRA and FishEye, supporting Subversion, Git and other types of VCS. One common use case for Crucible is to provide a workflow that transitions a ticket from open > under review > resolved. Another scenario is a post- commit review, done automatically after check-in of code changes. http://www.atlassian.com/software/crucible/
  2. Gerrit, http://code.google.com/p/gerrit/. Gerrit is a web based code review system, facilitating online code reviews for projects using the Git version control system.
  3. Checkstyle: this is not a code review tool, but more a developer tool to make sure his code is following standards and allow to save valuable time during peer code review.

And most important, use checkstyle to make code-checking a relatively simple task then you can make code review a regular activity and not something to be done at the end of a project, when deadlines are already making life miserable.

 

Enhanced by Zemanta

Submit a Comment

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>