Code Review Checklist - JU-DEV-Bootcamps/ERAS GitHub Wiki
As part of the code review process there are some points that the code has to cover, we have the next aspects to take into account:
General checklist
- Requirements
- Have the requirements been met?
- Have stakeholder(s) approved the change?
- Code Formating
- Is the code formatted correctly?
- Unecessary whitespace removed?
- Best Practices
- Follow Single Responsibility principle?
- Are different errors handled correctly?
- Are errors and warnings logged?
- Magic values avoided?
- No unnecessary comments?
- Minimal nesting used?
- Maintainability
- Is the code easy to read?
- Is the code not repeated (DRY Principle)?
- Is the code method/class not too long?
- Performance
- Is the code performance acceptable?
- Architecture
- Is it secure/free from risk?
- Are separations of concerned followed?
- Relevant Parameters are configurable?
- Feature switched if necessary?
- Testing
- Do unit tests pass?
- Do manual test plans pass?
- Has been peer review tested?
- Have edge cases been tested?
- Are invalid inputs validated?
- Are inputs sanitised?
- Documentation
- Is there sufficient documentation?
- Is the ReadMe.md file up to date?
- Other
- Has the release been annotated (GA etc)?
C# code review checklist (Microsoft Sugggested)
- Does this code make correct use of asynchronous programming constructs, including proper use of await and Task.WhenAll including CancellationTokens?
- Is the code subject to concurrency issues? Are shared objects properly protected?
- Is dependency injection (DI) used? Is it setup correctly?
- Are middleware included in this project configured correctly?
- Are resources released deterministically using the IDispose pattern? Are all disposable objects properly disposed (using pattern)?
- Is the code creating a lot of short-lived objects. Could we optimize GC pressure?
- Is the code written in a way that causes boxing operations to happen?
- Does the code handle exceptions correctly?
- Is package management being used (NuGet) instead of committing DLLs?
- Does this code use LINQ appropriately? Pulling LINQ into a project to replace a single short loop or in ways that do not perform well are usually not appropriate.
- Does this code properly validate arguments sanity (i.e. CA1062)? Consider leveraging extensions such as Ensure.That
- Does this code include telemetry (metrics, tracing and logging) instrumentation?
- Does this code leverage the options design pattern by using classes to provide strongly typed access to groups of related settings?
- Instead of using raw strings, are constants used in the main class? Or if these strings are used across files/classes, is there a static class for the constants?
- Are magic numbers explained? There should be no number in the code without at least a comment of why this is here. If the number is repetitive, is there a constant/enum or equivalent?
- Is proper exception handling set up? Catching the exception base class (catch (Exception)) is generally not the right pattern. Instead, catch the specific exceptions that can happen e.g., IOException.
- Is the use of #pragma fair?
- Are tests arranged correctly with the Arrange/Act/Assert pattern and properly documented in this way?
- If there is an asynchronous method, does the name of the method end with the Async suffix?
- If a method is asynchronous, is Task.Delay used instead of Thread.Sleep? Task.Delay is not blocking the current thread and creates a task that will complete without blocking the thread, so in a multi-threaded, multi-task environment, this is the one to prefer.
- Is a cancellation token for asynchronous tasks needed rather than bool patterns?
- Is a minimum level of logging in place? Are the logging levels used sensible?
- Are internal vs private vs public classes and methods used the right way?
- Are auto property set and get used the right way? In a model without constructor and for deserialization, it is ok to have all accessible. For other classes usually a private set or internal set is better.
- Is the using pattern for streams and other disposable classes used? If not, better to have the Dispose method called explicitly. Are the classes that maintain collections in memory, thread safe? When used under concurrency, use lock pattern.
Angular code review checklist
- Component Architecture
- Are components following the single responsibility principle?
- Is there a clear separation between smart (container) and presentational components?
- Are components kept reasonably small and focused?
- Template Syntax
- Is the template syntax clean and easy to read?
- Are structural directives (*ngIf, *ngFor) used appropriately?
- Is there any logic in templates that should be moved to the component class?
- Data Binding
- Is property binding [] used correctly for input properties?
- Is event binding () used properly for outputs?
- Are two-way bindings [(ngModel)] used sparingly and appropriately?
- Dependency Injection
- Are services properly injected and used?
- Is the dependency injection hierarchy logical?
- Are providers declared at the appropriate level (component, module, or root)?
- Modules
- Is the application properly modularized?
- Are feature modules used to organize related functionality?
- Is lazy loading implemented for larger modules to improve initial load time?
- Routing
- Is the routing configuration clean and well-organized?
- Are route guards (CanActivate, CanDeactivate) used where necessary?
- Is route data and resolvers used effectively?
- Forms
- Is there a consistent approach to form handling (Template-driven vs Reactive)?
- Are form validations implemented and working correctly?
- Is error handling for forms user-friendly?
- State Management
- For complex apps, is a state management solution (NgRx, NGXS, Akita) used consistently?
- Are actions, reducers, and effects (if applicable) well-defined and following best practices?
- RxJS Usage
- Are observables used effectively and unsubscribed properly to avoid memory leaks?
- Is the async pipe utilized in templates where possible?
- Are RxJS operators used efficiently for data transformation and event handling?
- Performance Optimization
- Is change detection strategy set to OnPush where appropriate?
- Are heavy computations memoized or moved to pure pipes?
- Is trackBy function used with *ngFor for large lists?
- Error Handling
- Are errors gracefully handled and logged?
- Is there a global error handling strategy?
- Are HTTP errors handled gracefully?
- Are user-facing error messages clear and helpful?
- Testing
- Are there unit tests for components, services, and pipes?
- Are integration tests in place for critical user flows?
- Is TestBed used correctly in setting up test environments?
- Styling
- Is there a consistent approach to styling (CSS, SCSS, CSS-in-JS)?
- Are styles properly scoped to components?
- Is the overall styling structure maintainable and following Angular best practices?
- Internationalization (i18n)
- Are all user-facing strings properly internationalized?
- Is the Angular i18n system or a third-party library used consistently?
- Accessibility (a11y)
- Are ARIA attributes used where necessary?
- Is the application navigable via keyboard?
- Do all interactive elements have proper focus management?
- Code Style and Formatting
- Does the code adhere to the Angular style guide?
- Is the codebase consistently formatted? (Consider using tools like Prettier)
- Are naming conventions clear and consistent?
- Documentation
- Are complex logic or algorithms properly commented?
- Is there sufficient documentation for custom services and components?
- Are public APIs of services and components well-documented?
- Build and Deploy Configuration
- Is the Angular CLI used effectively for development, testing, and production builds?
- Are environment-specific configurations properly set up?
- Security
- Is user input sanitized to prevent XSS attacks?
- Are security best practices followed for handling sensitive data?
- Dependencies
- Are all dependencies necessary and up-to-date?
- Is there a strategy for managing and updating dependencies?
General Proposal
As a proposal I think the checklist for Code Review should be the next:
- Have the requirements been met?
- Is the code easy to read?
- Do unit tests pass?
- Is the code formatted correctly? If the code complies with all the points, it is considered valid.
Angular Proposal
- Are components following the single responsibility principle?
- Is the routing configuration clean and well-organized?
- Are form validations implemented and working correctly?
- Are errors gracefully handled and logged?
- Is there a consistent approach to styling (CSS, SCSS, CSS-in-JS)?
- Is user input sanitized to prevent XSS attacks?
- Are all dependencies necessary and up-to-date?
C# Proposal
- Does this code make correct use of asynchronous programming constructs, including proper use of await and Task.WhenAll including CancellationTokens?
- Does the code handle exceptions correctly
- Is the code subject to concurrency issues? Are shared objects properly protected?
- There are no complex long boolean expressions (i.e; x = isMatched ? shouldMatch ? doesMatch ? blahBlahBlah).
- There are no negatively named booleans (i.e; notMatchshould be isMatch and the logical negation operator (!) should be used.
- Are internal vs private vs public classes and methods used the right way?