Monday, August 30, 2010

Good code reviews are inversely proportional to WTFs/minute in meeting

In my company where i am working , has just started peer code reviews. We conduct them as Email-Pass-around reviews and invite the team to participate in the meeting to gain a better understanding of the changes.
We use Source Control software that requires check-in, code-review rules to be signed off. Nothing big, just another developer's name that has reviewed the code.


There are definite benefits to code review as several studies have been able to demonstrate. in company, it is evident that code quality increased as the number of support calls decreased and the number of reported bugs decreased as well. The benefits of code review can be a more stable product, more maintainable code as it applies to structure and coding standards, and it allows developers to focus more on new features rather than “fire-fighting” bugs, and other production issues. 


There really aren’t any drawbacks if code reviews are conducted “right”. More on the “right way” below.
• Some of the hurdles to overcome while implementing code reviews are the idea that “big brother” is watching me and the idea that not having perfect code means torture and pain. Getting developers to trust each other is difficult sometimes, especially when it involves “pecking order” or the “holier than thou” attitudes and putting your hard work under a microscope. Trust is the key to resolving these issues. A developer must trust that they will not be punished by peers or management for mistakes in code. It happens to everyone. Make a note of the issue, get it resolved and move on.
Scrum One of the benefits of using the Scrum methodology is that a development cycle (”sprint”) is short. As the development iterations are smaller, the code review process reviews smaller pieces of code, making the review more likely to find problems than hours or days of formal reviews.
“Right Way” Code Reviews done the “right way” is completely subjective. However, I personally believe that they should be informal reviews. All of the participants in a review should avoid personally attacking the person being reviewed with statements such as “why did you do it that way?” or “what were you thinking?” etc. These types of comments diminish the trust between peers, leading to animosity, hours of arguing over the best/right way to code a solution. Keep in mind that developers do not think or code exactly the same, and there are many solutions to a problem. Just a little clarification on over-the-shoulder reviews; these can be conducted via remote desktop sharing (pick flavor here), or in person. However, they shouldn’t be limited to the developers only. We typically invite our entire team which consists of 10-12 developers.


Suggestion I would highly recommend code reviews.Create the basic rules for each role and implement them as part of culture to achieve a better quality product. It must become part of your culture so that it is part of a natural process and integrated at all levels, as it is a paradigm shift from poor quality, missed deadlines and frustration to better quality products, less frustration, and more on-time deliverables.


In general, I look for the following in a code review:-
  • Are all methods in a class as short as they could be
  • Do they have descriptive names
  • Are those methods doing only one thing (actually the thing that their name says it does)
  • Did somebody try to justify crappy code with some nice comments
  • How well does the code read in general
In addition we look at things like:-
  1. coupling
  2. cohesion
  3. thread safety (if threads were used)
  4. Boundary Conditions (what is going into and out of a method is what you expect e.g. no nulls)
  5. Code style follows company standards
  6. Code is not duplicated or copied (this can be automated Google for Simian)
  7. Code complexity - Google cyclomatic complexity
  8. Will the code that has been written cause any knock ons in other parts of the system
  9. If class, method and variable names are kept meaningful a lot of comments should not be necessary
  10. Is exception handling in place and comply with company standard
  11. There are Unit Tests for the code that are meaningful and the tests are relevant
  12. Logic errors
  13. Conformance to the specification (you have one of those, right?)
  14. Robustness/defensive programming

    No comments:

    Post a Comment