My Code Review Criteria

As a Scrum Master, I do code reviews. Recently, someone on my team asked whether what I look for in a code review was documented anywhere. This post is my starting point for documenting what I look for. I am hopeful that someone reading this may point out things that I haven't thought of that would be good to add to the list.

Backed by automated test
This should be self explanatory. No conditional logic or otherwise observable behavior should be added to the system without an automated test that confirms the predicable flow of the code.

Reinvents the Wheel
Does the code recreate some function that exists in a library included in the code base (or perhaps something from a utility library)? The chances of this are higher if you are reviewing code written by a developer new to the team. Does the code recreate a micro-framework or design pattern for which supporting types already exist and can be reused?

Doppelganger
Does the code contain logic duplication from elsewhere in the source file or from other files in the code base? Are there refactoring opportunities to extract code under review into a block reusable within the source file?

Maintains Harmony
Does the code under review look like it was written by the same hands as the rest of the code in the source file? In my mind, all source should read as if it was written by a single diligent developer, no matter the size of the team responsible for the system. That said, a projects' code style is an issue for teams to agree on internally and be ever vigilant for its application.

Says what it is / Says what it does
Are variables, constants, methods, and types descriptively named? Are any names inappropriately abbreviated or otherwise confusing?

Tell, don't ask
Classes and other types should accept their dependencies (through injection) rather than calling out for them (either via instantiation or other means). Accepting dependencies simplifies the ability to test code as well as change implementations of the types supplied at runtime. Does the code provide some means for external dependencies to be handed to the object? Are those external dependencies specified as interfaces or other non-concrete type declarations?

Too many notes
Are there any obvious opportunities for refactoring to simplify or reduce the size of the code under review? Does each method fit entirely within the view screen? The Single Responsibility Principle says that each type in a system should have one and only one reason to change. Are any methods, functions, or types written with multiple responsibilities?

Does it work?
Finally and most importantly, does running the system or related tests demonstrate that the code under review functions as expected?