Code review

Code review guide lines
11_Best_Practices_for_Peer_Code_Review.pdf

Which things instantly ring alarm bells when looking at code?
Q)

I attended a software craftsmanship event a couple of weeks ago and one of the comments made was "I'm sure we all recognize bad code when we see it" and everyone nodded sagely without further discussion.

This sort of thing always worries me as there's that truism that everyone thinks they're an above average driver. Although I think I can recognize bad code I'd love to learn more about what other people consider to be code smells as it's rarely discussed in detail on people's blogs and only in a handful of books. In particular I think it'd be interesting to hear about anything that's a code smell in one language but not another

Ans)

When grading a student's program, I can sometimes tell in a "blink"-style moment. These are the instant clues:

• Poor or inconsistent formatting
• More than two blank lines in a row
• Nonstandard or inconsistent naming conventions
• Repeated code, the more verbatim the repeats, the stronger the warning
• What should be a simple piece of code is overly complicated (for example, checking the arguments passed to main in a convoluted way)
• unreachable code
• commented code

Rarely are my first impressions incorrect, and these warning bells are right about 95% of the time. For one exception, a student new to the language was using a style from a different programming language. Digging in and re-reading their style in the idiom of the other language removed the alarm bells for me, and the student then got full credit. But such exceptions are rare.

When considering more advanced code, these are my other warnings:

• The presence of many Java classes that are only "structs" to hold data. It doesn't matter if the fields are public or private and use getters/setters, it's still not likely part of a well thought-out design.
• Classes that have poor names, such as just being a namespace and there's no real cohesion in the code
• Reference to design patterns that aren't even used in the code
• Empty exception handlers without explanation
• When I pull up the code in Eclipse, hundreds of yellow "warnings" line the code, mostly due to unused imports or variables
• Check the scope of access modifiers.

In terms of style, I generally don't like to see:

• Javadoc comments that only echo the code

These are only clues to bad code. Sometimes what may seem like bad code really isn't, because you don't know the programmer's intentions. For instance, there may be a good reason that something seems overly complex-- there may have been another consideration at play.