My Pull Request Review Checklist — to perform effective Code Reviews

Credits: Stack Exchange

Code review is systematic examination of computer source code. It is intended to find mistakes overlooked in software development, improving the overall quality of software. Reviews are done in various forms such as pair programming, informal walkthroughs, and formal inspections.

A code review is a process where developers visually inspect a set of program code.

One part of managing a software-engineering process is catching problems at the “lowest-value” stage — that is, at the time at which the least investment has been made and at which problems cost the least to correct. To achieve such a goal, developers use “quality gates”, periodic tests or reviews that determine whether the quality of the product at one stage is sufficient to support moving on to the next.

— McConnell

The original intent behind code reviews are that they would help us to take collective ownership in the creation of our software. Without a Code Review checklist, it’s too easy to miss whether code is doing perfectly or not. Here comes my Checklist:

General

• Does the code work? Does it perform its intended function, the logic is correct etc.
• Is all the code easily understood?
• Does it have correct linting?
• Is there any redundant or duplicate code?
• Is the code as modular as possible?
• Can any global variables be replaced?
• Can any of the code be replaced with library functions?
• Can any logging or debugging code be removed?
• Does code follow defined architecture?
• Does feature implementation follow UAC and specified design?
• Ensure PR passes CI server, re-running once or twice if it fails. Failure could be due to other issues, so determine whether the PR is responsible if failure occurs.
• Request changes to the code and/or additional unit tests if any issues are found.
• Is any code invoking memory leaks?
• Are properties declared with the correct storage semantics? (e.g. weak or unowned instead of strong)
• Are good names used for classes, enum, struct , methods, and variables?

Documentation or PR explanation

• Is any edge-case handling described?
• Is the use and function of third-party libraries documented?
• Are data structures and units of measurement explained?

Testing

• Is the code testable? i.e. don’t add too many or hide dependencies, unable to initialize objects, test frameworks can use methods etc.
• Do tests exist and are they comprehensive? i.e. has at least your agreed on code coverage.
• Do unit tests actually test that the code is performing the intended functionality?
• Are arrays checked for ‘out-of-bound’ errors?
• Could any test code be replaced with the use of an existing API?

Some points added by Abhishek Neb

• If using any 3rd party library, check for licenses and compliances
• If storing any sensitive information, it must be stored at secure place with encryption technique like HASH STRETCHING or something else.
• Avoid Code smells and try to follow Clean Code book concepts by Robert C. Martin.

If I missed something in checklist, write down in comments.