Code Review Review

Programmers write code and code review is often required by managers to be done prior to code submission. I tend to dislike the process since there's a tendency for reviewers to place an emphasis on certain trivial problems and little attention is made to important qualities.

Here's my list of what a code reviewer should look for, from most important to least important:

  1. Validation by additional or changed unit tests.
  2. Duplication of functionality available elsewhere. Ironically, a lot of programmers like to rewrite things, even though it's obviously more work for them.
  3. Sections of code or idioms repeated.
  4. Clarity in what the new functionality does.
  5. All code is clearly required. Programmers love to make things that "might be useful someday" and usually it's never used. In Java, I often see people implementing "equals/hashCode", serialization, etc. for classes that never use these methods.
  6. Signs of junior programming practice: "Utility" classes, "Constant" classes, multiple layers (see 5), singletons, and other "Design Patterns Book" nonsense, etc. Note that utility classes aren't really bad per se, it's just that they are often written instead of supplying the appropriate functionality to an existing class or writing a new class.
  7. Untyped or too many parameters. Often the same sequence of parameters appear in multiple places, see 3.
  8. Premature optimization. See: 2. Actually this doesn't really matter as much, assuming this optimization is well tested.
  9. Class names that repeat the current namespace. Names that are too long. See 3.

The above common problems are pretty easy to spot. The longer you spend on code reviews and generating comments the more it seems you're being overly critical, so always comment on the most important areas only.

Here are things that aren't as important and almost worth ignoring:

  1. Code (in)correctness. It's fairly impossible to find bugs just by reading code and unit testing is hopefully going to prove the code is correct. If you do spot some problem, mention it, I just don't see why you should try to "run" the program in your brain, that's what automated testing is for.
  2. Missing boiler-plate information, such as legal or copyright notices. This can be corrected using a tool, if needed. We eventually stopped adding boiler-plate to code at m-Qube since our code wasn't shared.
  3. Lack of code comments. The problem with most comments is they take up space, are often wordy, and don't explain anything new. Comments grow more and more inconsistent over time, so it's almost better to leave them out until the end of a project.
  4. Improper formatting. Batch tools exist to fix formatting, so there's no need to address formatting concerns in the middle of a project. But since converting line endings causes trouble for code merging, if you see line endings change mention it.
  5. Lack of parameter checking. It's a good idea to check parameters when constructing objects, but especially in Java it's not so important for every function.
  6. Not following naming conventions. Some reason people I work with insist "enum' type names should end in "Type", but everything is a "Type" already so I don't see how it clarifies things. I also worked with people who wanted Java interfaces prefixed with "I", which is not done in the standard Java library. If 90% of programmers don't follow your naming strategy, then maybe it's not necessary.

Read and post comments | Send to a friend

Advertisements

About eliasross

Blogging before the word "blog" was invented.
This entry was posted in Uncategorized and tagged , , , , . Bookmark the permalink.

2 Responses to Code Review Review

  1. Jason Cohen says:

    Nice post.I especially like the point about having unit tests instead of "running the code in your head."

  2. Jason Cohen says:

    We review all our code (of course, since code review is what we do), and we've found that this process works well:1. Review the unit tests. Make sure they are complete but don't over-specify. A bug here means you probably don't even have to look at the code yet.2. Review the method doc. Since the true behavior is now fresh in your head (you read the tests), it's easy and quick to verify that the doc is right.3. Review the method, looking for things as in the first 1-9 above, not in the first 1-6. You don't need to check for method behavior since the tests work.I also agree that static checking is vital. No need to waste a human's time on things that can be automated.Thanks!P.S. My blog about running a software company, and a little about code reviews.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s