PR Approval Checklist

PR Acceptance Checklist

All code reviews should occur within Github's code review interface, allowing you to write comments directly on the appropriate line of code. Code should first be inspected in the code diff view to see what has changed. Second, the code should be pulled into a local branch, allowing you to review the altered code in full context.

Preliminary

• The project builds without errors or warnings.
• Unit tests pass.

General

• I am able to understand all of the code without difficulty. Variable, method, and class names follow convention and are clear.
• Code is formatted correctly.
• Code has not been duplicated (Don't Repeat Yourself).
• Proper logs have been added.
• No debug specific code remains.
• No code can be replaced with library methods.
• Global code (e.g. singletons) has been avoided as much as possible.
• Values are not hard coded, but exist in configurations / constants.
• Performance is acceptable and cannot be easily and obviously improved.
• Design patterns have been used appropriately (e.g. Dependency Injection and Facade are two common patterns).
• Code is flexible, scalable, and maintainable. It can be easily added to and modified without requiring major revisions and API changes. A component could be replaced with a better one without much difficulty.
• Each unit of code does one thing and one thing only (it follows the Single Responsibility Principle). No class or method is obviously too large.
• Separation of Concerns is followed. Code is split into multiple tiers / layers as necessary.
• NS_ASSUME_NONNULL_BEGIN / NS_ASSUME_NONNULL_END tags exist in all .h and .m files.

RPC Updates

• New files are placed into the public folder on the file system.
• New files are imported in SmartDeviceLink.h.

Comments

• There are no remaining TODOs or FIXMEs. HACKs are clearly labelled. There is no commented out code.
• Comments are used sparingly where the code is unclear, or unusual behavior / edge-cases exist. Comments are why, not what.
• Methods and classes (especially public ones) have API documentation explaining the what.

Automated Testing

• The code is designed for appropriate unit tests.
• There are appropriate and comprehensive unit tests to cover what can and should be covered by unit tests.
• If unit test coverage is deficient, appropriate unit tests have been suggested.
• The unit tests test the intended functionality.
• Tests should not cover built-in / 3rd party library functionality.

Smoke Testing

• Appropriate regression smoke testing has occurred. (TODO: Smoke test plan)
• Appropriate smoke testing which exercises the altered code / functionality has occurred. (TODO)