What To Look For In A Code Review?
Published: 2023-08-18 08:36:54


### 1. Implementation

- Does the code change achieve its intended purpose?
- Are there any unwanted dependencies added?
- Are any unnecessary frameworks, APIs, libraries, or services used?
- Could the code be improved by using a different approach?
- Is the code at the appropriate abstraction level?
- Is the proposed solution well-designed and usable?
- Is the API well-documented and intuitive to use?

### 2. Data Structures

##### 2.1. Lists

- Anti-Pattern: is too much searching used?
- Anti-Pattern: is there frequent reordering?

##### 2.2. Maps

- Anti-Pattern: is the map used as a global constant?
- Anti-Pattern: is there iteration and reordering?

##### 2.3. Sets

- Anti-Pattern: do you need to have duplicates?

##### 2.4. Stacks

- Anti-Pattern: is iteration used over the stack?

##### 2.5. Queues

- Anti-Pattern: is iteration used over the queue?
- If unbounded queue is used, how does it affect the performance?
- If bounded queue is used, can the queue become full and what could happen to the system?

### 3. SOLID Principles

- Does this code adhere to SOLID principles?
- Does similar functionality already exist in the codebase? If yes, why isn’t it reused?
- Are there any design patterns or language-specific patterns that could improve the code?

### 4. Error Handling and Logging

- Can the code behave unexpectedly in any use case?
- Are there any inputs or external events that could cause the code to fail?
- Is error handling done correctly?
- Should any debugging information be added or removed?
- Are there enough log events for easy debugging?

### 5. Security and Data Privacy

- Is (user) input validated, sanitized, and escaped to prevent security attacks
  such as cross-site scripting or SQL injection?
- Were updates to documentation, configuration, or readme files made as required by this change?
- Are there any potential impacts on other parts of the system or backward compatibility?
- Does the code introduce any security vulnerabilities?
- Are authorization and authentication handled correctly?
- Does your data need to be encrypted?
- Is the right encryption used?
- Are secrets being managed correctly?
- Does this code change reveal any secret information like keys, passwords, or usernames?
- Is sensitive data like user data or credit card information securely handled and stored?
- Is data retrieved from external APIs or libraries checked for security issues?
- Should the code be logging/auditing behaviour? Is it doing so correctly?

### 6. Tests

- Are there tests for the new code?
- Do the tests at least cover confusing or complicated sections of code?
- Are the tests easy to understand?
- Do the tests align with the requirements?
- Are there any cases that are not covered by the existing tests?
- Are there tests to document the limitations of the code?
- Is the code covered by appropriate test types and coverage?
- Are there tests for security aspects?

### 7. Performance

##### 7.1. Performance Requirements

- Does this piece of functionality have hard performance requirements?
- Could this code change negatively impact system performance?
- Has the fix/new functionality negatively impacted the results of any existing performance tests?
- Is there potential to improve the code's performance?

##### 7.2. Calls outside of the service/application are expensive

- Are there any calls to the database?
- Are there any unnecessary network calls?

##### 7.3. Using resources efficiently and effectively

- Is there something in the code which could lead to a memory leak?
- Is there a possibility the memory footprint of the application could grow infinitely?
- Does the code close connections/streams?
- Are resource pools correctly configured?
- If there is reflection, is it absolutely required?
- If there are timeouts, what’s the impact on the rest of the system while this timeout is ticking down?
- Does using multiple threads for a simple operation make the code slower and more complex instead of improving
  performance?

##### 7.4. Correctness

- Is the code using the correct data structure for a multi-threaded environment?
- Is the code susceptible to race conditions?
- Is the code using locks correctly?
- Is the performance test for the code valuable?
- If the code contains caching mechanism, are there any instances where cache items are incorrectly invalidated?

##### 7.5. Code level optimisation

- Does the code use synchronization/locks when they’re not required?
- Is the code using a thread-safe data structure where it’s not required?
- Is the code using a data structure with poor performance for the common operations?
- Could the code be using atomic variables instead of locks or synchronization?
- Could the code benefit from lazy loading?
- Can if statements or other logic be short-circuited by placing the fastest evaluation first?
- Is there a lot of string formatting? Could this be more efficient?
- Are the logging statements using string formatting? Are they either protected by an if to check
  logging level, or using a supplier which is evaluated lazily?

### 8. Readability

- Is the code easy to understand?
- Can the code's readability be improved by smaller methods or different function/variable names?
- Are there redundant comments?
- Could some comments be removed by making the code itself more readable?