Code Review – Quick Reference
This is a simple check-list to make code reviews more valuable. Simply check these rules.
Download a single page word document: Code Review Cheat Sheet
What % of code reviews should pass the first time?
About 10% should pass the first time. The other 90% of the time, a code review should fail. Now this low 10% percentage of first-time successful code reviews might increase as a team matures and members follows these rules as a behavior, but if you start to see really high first-time successful reviews, you might need to re-focus on this article.
However, if 100% or even 80% of code reviews are passing without comments, then you are likely not code reviewing correctly.
Does the code follow the 10/100 Rule?
This is a quick check rule that isn’t extremely rigid. See the 10/100 rule of code
This rule is what I call the S.O.L.I.D. training wheels. For more on S.O.L.I.D., see below. Code often follows all other rules naturally if it follows the 10/100 Principle.
Method has less than 10 lines
Is the method that was added or changed 10 lines or less? (There are always exceptions such as Algorithms)
100
Is the class 100 lines or less?
Note: Model classes should have zero functions and be closer to 20 lines.
Existing code that breaks the 10/100 Rule
Did the class get smaller? We don’t make it bigger.
Is the code S.O.L.I.D.
S.O.L.I.D. is an acronym. See this link: https://en.wikipedia.org/wiki/SOLID
Single Responsibility Principal
Does each class have a single responsibility? Does each method have a single responsibility?
Is this the only class that has this responsibility? (No duplicate code or D.R.Y. (Don’t Repeat Yourself)
Open/Closed Principle
Can you extend the functionality without modifying this code? Config, Plugins, event registration, etc.
Is there configuration in this code? If so, extract it. Configuration does not belong in code.
Encapsulation. Is everything private or internal except the few things that should be published as public?
Liskov substitution principle
Is inheritance used? If so, does the child type cause issues the parent type wouldn’t cause?
Interface segregation principle
Does the code use interface-based design?
Are the interfaces small?
Are all parts of the interface implemented without throwing a NotImplementedException?
Dependency inversion principle
Does the code reference only interfaces and abstractions?
Note: If new code references concrete classes with complex methods, it is coded wrong.
Cyclomatic Complexity
Cyclomatic Complexity is the number of linearly-independent paths through a program module. Basically, if statements, conditions, etc. Higher is bad.
Are there too many conditions or branches in a piece of code? If so, that can make it buggy. Usually you never have high Cyclomatic Complexity if you follow the 10/100 Principle.
Is the code Unit Tested
99% coverage
Is the Code 99% covered? Is code not covered marked with the ExcludeFromCodeCoverageAttribute?
Test Names
Are tests using proper names: <ClassName>_<MethodName>_<State>_<Result>
AAA Pattern
Tests should be written with Arrange, Act and Assert parts clearly marked, often with comments.
Parameter Value Tests for methods with parameters
Are all parameter values that could cause different behavior covered?
See these links:
Unit testing with Parameter Value Coverage (PVC)
Parameter Value Coverage by type
Test Example
Example in C#: Imagine a class called MyClass with a method: string DoSomething(string s);
public void MyClass_DoSomething_ParameterSNull_Throws() { // Arrange var myClass = new MyClass(); var expected = "SomeExpectedValue"; // Act var actual = myClass.DoSomething(); // Assert Assert.AreEqual(expected, actual); }
Naming things
Typos
Are your names typo free?
Naming convention
Do your file names, class names, method names, variable names match existing naming conventions?
Specific Names
Are the names specific. Avoid general words “SomethingManager” or “AbcHelper”. General names should be avoided. Specific names are self-documenting.
Big O
Do you have any glaringly obvious Big O problems? n or n2 vs when it could be constant or log n.
See: https://en.wikipedia.org/wiki/Big_O_notation
String Handling
Are you handling strings safely?
Are there magic strings? Do you understand the difference between volatile (untrusted strings) and nonvolatile (trusted strings).
Documentation
Did you write documentation? Is every public method/property/field documented?
Self-review
Has the submitter reviewed their own code. I understand that some tools don’t allow you to submit a review for your own change. That is not what I am talking about. I am talking about you have reviewed your own code, added comments anticipated questions from reviewers, and made sure the code meets the code review check-list yourself.
You should be failing your own reviews once or twice before anyone else even has a chance to look at them. If the tool doesn’t support a self-review, then leave a comment, saying: I have reviewed this myself and it is ready for others to review.