Code Review
This is not original thinking
Most if not all of the following ideas are pulled from other sources. This is simply a distillation of those ideas that match my own personal experience or ring true to me.
Also; I want to document them somewhere and that somewhere is here.
The main objective of code review is not to catch bugs; it is to communicate ideas and enforce implicit design decisions.
Code review provides an opportunity to disseminate ideas and decisions made by an individual to the rest of the team. As complexity rises it is critical to have all team members be aware of how each piece of an application is evolving.
Terms
PR: “Pull Request”. A request to merge a feature branch (changes) into the active code base.
Author: The creator of the feature branch.
Reviewer: The person performing he code review.
Does the PR follow DRY principles?
DRY = Don’t Repeat Yourself. Simply put; does the PR contain duplicated code or functionality that is provided by other modules.
Does the PR follow SOLID principles?
SEE: https://scotch.io/bar-talk/s-o-l-i-d-the-first-five-principles-of-object-oriented-design
Single Responsibility Principle
A class should have one and only one reason to change, meaning that a class should have only one job.
Open-closed Principle
Objects or entities should be open for extension, but closed for modification.
Liskov substitution principle
Let q(x) be a property provable about objects of x of type T. Then q(y) should be provable for objects y of type S where S is a subtype of T.
OR
Every subclass/derived class should be substitutable for their base/parent class.
Interface segregation principle
A client should never be forced to implement an interface that it doesn’t use or clients shouldn’t be forced to depend on methods they do not use.
Dependency Inversion principle
Entities must depend on abstractions not on concretions. It states that the high level module must not depend on the low level module, but they should depend on abstractions.
Does the PR follow the project’s exiting patterns?
Does the PR following the pre-existing: naming conventions, scoping, patterns, etc? Note that the answer to this question will be different for every project.
Is the PR overly complex?
Does the PR contain functionality that exists elsewhere?
Is there a simpler solution that accomplishes the same goals or meets the same requirements?
Does the PR break backwards compatibility?
If yes; is there a valid/un-avoidable reason for doing so?
Does the PR change underlying data schema(s)?
If yes; is there a valid/un-avoidable reason for doing so?
Does the PR contain any performance risks?
Are there usages of known, poor performing, functions/methods/patterns?
Does the PR contain iterations within iterations?
Does the PR contain recursion?
Does the PR contain any security risks?
Are DB queries properly protected against SQL injection?
Is any user input sanitized before being passed into internal functions?
Does the PR contain any secrets that should not be committed to source control?
Are there vectors for abuse by malicious actors? CC scraping? Email address scraping? Data leakage?
Are proper encryption routines being employed?
Is any part of the PR not understandable?
Does the PR need refactoring for readability?
Does the PR need comments added to make that which is implicit explicit?
Does the PR contain supporting documentation?
If there are function DOCBlocks where they updated?
If there are wikis or external documents were they updated?
Does the PR contain references to the source issues?
Are there comments within the new code blocks that reference the issue which generated the changes in the first place?