On Code Reviews

I know I said in a previous post, that enough has been said about code reviews, but I’m writing about them. I may have to do more code reviews in the near future, so it was a good time for me to gather myself and think of all the principles I should keep in mind. Also, if anyone else has any thoughts on the matter, I would love to hear them.

Define Your Goals

This seems a little weird at first. “Our goal is to write good code.” Well yes and no. The group that I manage is in charge of administering the SQL, ColdFusion and IIS boxes. My personal goal with code reviews is to ensure I don’t get paged at 2am because our servers are down. I primarily look for performance and security issues. Another member of the code review team might manage a team of programmers; they might be looking at compliance with an accepted set of conventions, because their goal is to make sure that when changes need to be made, anyone on the team can quickly make them. Still another person on the code review team might be focused on usability as their goal is to comply with government regulations.

It’s okay that there might be more than one goal. Just figure out what your organization’s goals are, and shape your team’s goals accordingly. My team’s goals are:

1. To protect shared resources from performance problems due to inefficient code.
2. To show off new and interesting techniques
3. To learn from each other’s experiences

Define Standards

Now that you know what your goals are you need to come up with what the rules are. I start with the Adobe CFMX Coding Guidelines. I look at performance and security issues, and then start building out rules from there. Then I collect tidbits from other online sources, and start lumping them all together.

I need to say, before I started any of this, I read my copies of the ColdFusion MX 7 Web Application Construction Kit and Advanced ColdFusion MX 7 Application Development. These are so fundamentally important to writing good ColdFusion, that we don’t even bother telling people to read them. You start working for us, you get handed copies of them.

Publish Them

This seems to be a no brainer, but people really get annoyed when they come to a code review and are told they’ve violated our super secret standards. Now in our mind, they’re never secret. Everybody who is reading the blogs know what techniques are out of favor now right? Well no, because not everyone who codes in your organization keeps up to date with the ColdFusion blogosphere. So you need to tell them what they standards are. We do this by including a blog in our development center that has a category named “Best Practices.” We add to it as techniques or situations come up. We then force our developers to see the blog, because the rest of our development tools are on the same site.

Give Directions

We very tell people the following:

• Make sure your application conforms to current best practices
  • Read the Devcenter Best Practice Entries.
• Prepare CFML, HTML, CSS
  • Make a local copy of your code. Feel free to include stylesheets, but don’t bother with images.
  • Remove database, or other passwords from code.
  • Remove any unused templates
  • Clean any commented code out of your applications
  • Remove development diagnostic code like <cftimer> or <cfdump>
  • Remove database, or other passwords from code.
  • In short, code should be ready to go to production when reviewed.
• Prepare SQL
  • All SQL going against a database server must be called in a stored procedure.
  • Use Query Analyzer to properly index tables. [Link to tutorial]
  • Use Enterprise Manager to generate scripts for all tables, including indexes. [Link to tutorial]
  • Use Enterprise Manager to generate scripts for all stored procedures. [Link to tutorial]
• Prepare Flow Chart
  • Use Visio or some comparable program to chart the flow of your application.
  • Visio can be obtained from [Link to Central Installs location]
• Zip up all the collected files.

You may require other things. But make sure people are told what those things are.

Read the code before the review

Nothing irritates me quite as much as a reviewer who insists on not reading the code ahead of time. To force others to wait because you say “hold on” as you flip through pages is incredibly disrespectful to you co-workers.

Some people print out all of the code, and review directly on the paper. With the size of applications these days, this is becoming more and more unwieldy. I’ve taken to using my two monitors and putting up code in one window, and Microsoft One Note in the other. That way I can just record my thoughts, or copy and paste code. Additionally, I don’t waste paper.

Make clear and concise recommendations with references to specific lines of code. Here is a sample from one of my reviews:

Game

Application.cfc

Line224

Why do you raise the logout action to an application variable? Is there a good reason for doing so?

Application_security.cfc

Line 319-326

I really like this bit of logic.

effHistroyFunctions.cfc

Line 61

This is a bit confusing: either the object you are creating either exists throughout the entire CFC (which is only one function for now, but could grow at some point.) so it should be in the variables scope and init when the cfc init’s. OR it only exists in this function in which case it should initialized with a var.

Error_exception.cfm

Line 22

CompareNoCase is prefered over eq

1 eq 1 will always be true. So this conditional will always be true so get rid of it.

Error_request.cfm

Line 23

Using the error.mailto is preferred over using a hardcoded address.

Frontier.cfc

Function frontier_calc

I feel like this function might be a little hard to maintain and troubleshoot.

Additionally it has explicit steps within it. It could be encapsulated a little further to make troubleshooting and debugging easier.

Legend

• Must Change
• Suggested Change
• Stylistic Tip
• Curiosity, Not Criticism
• Comment

Notice also that we make allowances for several levels of severity. There are things that must be changed. But we also place an appropriate priority on less important changes so developers against a deadline know what they have to do, and what they can negotiate with us over.

The Review

The developer presents their application. They tell us what its purpose is, any specific challenges they faced, or what problem they are trying to solve. Then we usually go through the code technology by technology. (So Flex code, then ColdFusion code, then SQL, then XHTML/CSS etc.) The team gives our feedback, and then discuss any points of contention. (I think something is the best way to do it, but another review disagrees, and the developer has a specific reason for choosing to do it in a way other then what we think is “best.”)

Keep it courteous. Keep it respectful. And don’t forget to praise the good things you find.

Aftermath

The developer should make changes to their code based on the review. The reviewers aren’t done yet though. Was anything “widely accepted” as best practices new to a developer? Then it should be posted with your best practices; even if you have to repost it. You may also have to sit down with your fellow developer and help them learn a new technique.

Conclusion

Code reviews are a necessary part of running a good programming shop. No one should be exempt; even the makers of the standards. They can be painful, and ego bruising, but with a little preparation, the negatives can be downplayed, and the positives allowed to shine through.