Tools

Tips for Code Review Success

by on September 23, 2013 9:52 am

I’ve had the opportunity to be involved in various group peer code reviewing scenarios and sometimes I find myself wondering – is this worth it? (This is often followed by my drifting mind wondering if the cafeteria in whatever building has any more cookies, while a 20-minute-long off-topic conversation on the review sadly ensures that the cookies will be gone.)

Personally I think that yes, code reviews are often important to both help maintain a quality code base and, by doing so, also help limit production issues. Reviews aren’t always to find mistakes but also help find a better way (more reuse, more efficiency, more readability) to solve the issue, which improves the overall quality of the code. In addition to enhanced quality, an equally great goal of reviews is to help spread knowledge about the code base to the whole team.

Note: There are lots of arguments as to whether they are not needed in a pure Agile environment and I’m not really addressing that here. I’m assuming a team where some aspects of Agile may be used, but not for example pair programming, which limits the need greatly.

All of this is good, but code reviews can be costly in terms of developer time, so while in the right situations they can be great, it’s also important to keep them on task and limit the scope up front.

Here are some items to focus on to help make your code reviews effective:

Prep work should take place People should make a practice of already following the written coding standards for the team so that not too much time is taken up by worrying about things like project-specific naming conventions. It’s important that these are met and occasionally finding them in review makes sense, but can too much emphasis can pull everyone off task (there are not unlimited cookies people, seriously).

Focusing on things like formatting is also a sign that things are not on task. If there is a way to standardize something automatically in your IDE, then everyone should absolutely be doing that. It shouldn’t need to be mentioned in the review.

In addition, making it a practice to use static code analysis tools like FindBugs and code coverage tools like Cobertura to ensure test coverage will help find issues that may have been missed and help keep reviews short and focused. This leaves the reviewers to focus on more subtle logic and design issues, etc.

Keep it short and frequent Reviewing takes a great deal of concentration and longer periods can lead tiring and missing issues. Plus, it does take away from development time, so of course it’s helpful to stay quick and on task.

It’s best to have the changes fresh in everyone’s head. There are lots of ways to do reviews and what is best is going to vary by team and project, but whether you do over the shoulder, pair, send your code for review by tool or do a more formal review in a group, it is best to keep them frequent and small. Doing them more often means that major issues can be resolved before they have gone too far.

Everyone plays Even the most advanced architect or super star developer can make mistakes. It’s so much better to catch them here. Plus, again, it’s a learning opportunity for more junior people and people that are newer to the application.

Constructive Input General comments like “more comments” are generally, well, less than helpful. An interface with no comments might be an exception here, and documentation is good, but if you see something that doesn’t make sense this is the time to figure out why it’s confusing and determine if the design can be improved or if an error in the logic exists.

Focus on truly understanding what the code does and ensuring that if someone else needs to maintain it, they can understand it.

Tools Use tools designed for code reviews to speed and ease the process. For example, Crucible is the popular Atlassian code review tool which provides a lot of great reviewing features.

Checklists There is probably no need to go overboard on these or to be too structured, but if certain issues keep cropping up, having a checklist can be a handy tool.

Positive Environment However many people are involved, make it a positive team effort to share information and create the best application possible.

Happy reviewing. Now, go get that cookie and get back to developing.

— Adrienne Gessler, asktheteam@keyholesoftware.com

  • Share:

6 Responses to “Tips for Code Review Success”

  1. Dave says:

    You touch on a lot of good points, especially the prep work and positive environment. This is a good article for those groups starting to implement code review, or those looking to re-evaluate their current code review process.

    To supplement your comprehensive article, I would mention only two other things.

    First, only programmers should be in the code review. Management can hear these sorts of discussions as a set of imminent crises. Give them a summary afterward.

    Second, code reviews should stay positive and no individual should feel attacked. It hurts the constructive nature of the review. For example, I know of one shop that used code reviews as a means to analyze developer performance. Sure, it’s really easy to go through Crucible and determine how many comments each person has on their review and try to use that as a yardstick. Not only is it largely inaccurate (are all the comments valid or even issues? etc.), but it ultimately discourages the team from doing code reviews.

    Thanks!

  2. Lou Mauget says:

    This helpful post is timely for my current project at Railinc. I’ve forwarded it to our developers. David’s two points are excellent too.

  3. Tash Mahmood says:

    You have made a great case for code reviews. I like your point about Fisheye/Crucible, but I would add that we have used the code quality platform “Sonar” for our code reviews. It allows us to do automated as well as manual code reviews. Coding and reviewing are components of a cyclical process. Violations of standards should not only come from a peer code review, but it can be identified at any time. You can add these violations to an automated code review so that Best Practices of App Dev are always considered.

    Additionally, Whereas management is not involved in the code review itself, but it needs to be aware of the tasks that come out of a code review. Management MUST take interest in code quality!

Leave a Reply

Things Twitter is Talking About
  • We <3 consulting & we <3 development! Want proof? We created #GrokOla, a tool for clients to ask our team high-level development questions.
    April 26, 2015 at 4:04 PM
  • Want to write a single page app with ExtJS? View @zachagardner's video tutorial series for guiding #ExtJS principles: http://t.co/q4ctOYduNb
    April 25, 2015 at 12:47 PM
  • It's Friday! Nothing better than an impromptu Keyhole team happy hour @75thStBrewery. Great way to kick off the weekend. #loveourteam
    April 24, 2015 at 4:00 PM
  • .@Jinaljay transitioned to #Java from .NET & needed to switch IDEs. Tips that helped her in the switch to #eclipse - http://t.co/wbx9qGcbhX
    April 24, 2015 at 2:30 PM
  • Do you like to lay #JavaScript code just as much as you love to manage dev teams? Apply to be a Lead JS Dev with us - http://t.co/TIKNFUZj4q
    April 24, 2015 at 12:16 PM
  • Know Your IDE: #Eclipse http://t.co/wbx9qGcbhX With tips and shortcuts to make it easier to use.
    April 24, 2015 at 11:27 AM
  • #Java is OO but contains non-object primitives. Autoboxing feels more like a band-aid. Do primitives need to go? http://t.co/A8ChCBqmle
    April 24, 2015 at 10:59 AM
  • .@racingcow we're so glad you're interested in attending #KCDC15! Just follow us and we will DM you the code to save 10% off your ticket!
    April 23, 2015 at 4:25 PM
  • Rapid #appdev has a bad rep, but there are ways to bring development time down the right way. Don't Fear the Rapid - http://t.co/8CKhAzE9jJ
    April 23, 2015 at 2:30 PM
  • #JavaScript debugging is tough - every technique you have in your toolbelt can help! Time-Oriented #Debugging - http://t.co/n0JrRPrWHt
    April 23, 2015 at 10:20 AM
  • 4 Creative Ways to Test Your Code in Production - http://t.co/OijrRetTNA
    April 23, 2015 at 9:13 AM
  • Code For Maintainability So The Next Developer Doesn't Hate You - http://t.co/iG2wW2rSWj Eight helpful tips to do so.
    April 23, 2015 at 6:30 AM
  • Big news - we've released a demo version of #GrokOla open to the public. Try out its features & capabilities - http://t.co/O4ladoeLhk
    April 22, 2015 at 2:50 PM
  • Here's a free primer for ​implementing a RESTful API using #SpringMVC ​- http://t.co/JWvqeg3SST #GrokOla
    April 22, 2015 at 2:20 PM
  • Interesting article with a neat open source tool - osquery. Why Sharing Your Security Secrets Is a Good Thing http://t.co/Tcpt27ibjI
    April 22, 2015 at 8:50 AM
  • ICYMI: Free #GrokOla development primer. Get to know #Java Lambdas - http://t.co/D2iLLj8mph
    April 21, 2015 at 4:31 PM
  • We've been releasing some free #Grokola tutorials. See them all in one spot - http://t.co/WDt5fVSwaA #JavaScript #Backbonejs #nodejs #java
    April 21, 2015 at 2:30 PM
  • Are you a planner? Then you'd better get your @kc_dc tickets! Tweet us for a '10% off your ticket' #KCDC15 code. We're excited to sponsor!
    April 21, 2015 at 12:15 PM
  • #Netty can handle multiple traffic types concurrently, like #WebSockets & HTTP over the same port at the same time: http://t.co/TIufQDCCiC
    April 21, 2015 at 10:04 AM
  • There's a new post on the Keyhole blog from @Jinaljay - Know Your IDE: #Eclipse http://t.co/wbx9qFUAqp
    April 20, 2015 at 10:27 AM
Keyhole Software
8900 State Line Road, Suite 455
Leawood, KS 66206
ph: 877-521-7769
© 2015 Keyhole Software, LLC. All rights reserved.