1 post tagged “code 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.