Pull Request reviews and checkpoints to success

Being part of the digital agency as a Developer, I've been working in multi-cultural teams across the world, and found different personas and developers when it comes to the point of the PR review.

It makes a lot of importance to compose yourself, take your time, and review done changes, because no one is perfect, everyone makes mistakes, and done changes should be bulletproof for the rest of the features — and enable you to reuse them as one of the most done modular features.

Through this blog I will let you through 21 checkpoints, which all of you may or may not consider important. It's all about team setup, team consolidation, and finally conversation on the pull request — to go through, or debate about possible changes. In the end, both reviewer and PR creator are working for the same team, same goal.

1. Duplicated code

One of the most common smells in code because it's too easy to copy code. Code duplication can be a silent enemy that creeps into our projects, leading to maintainability issues, increased bug potential, and wasted developer effort. This is the critical phase where the team identifies and resolves code duplication issues.

• Codebase health: eliminating duplicated code improves the overall health of the codebase. It reduces the risk of inconsistent behavior and simplifies future maintenance.
• Efficiency: duplication is often a source of inefficiency. By centralizing functionality, we optimize resource usage and reduce execution time.
• Readability: consolidated code is easier to read and understand, making it simpler for developers to work with and maintain.
• Bug reduction: duplicated code is a breeding ground for bugs. Removing redundancy reduces the likelihood of introducing new issues.

2. Long methods

Methods longer than 20-30 lines (actually with more than one responsibility). Long methods can lead to reduced code readability, increased complexity, and difficulties in debugging and testing. This review process is designed to identify, refactor, and optimize long methods to improve overall code health.

• Improved code readability: smaller methods are easier to read and understand, making the codebase more accessible.
• Enhanced maintainability: breaking down long methods simplifies maintenance and reduces the risk of introducing bugs.
• Modularity: smaller methods can be reused in various parts of the codebase, promoting modularity and reducing redundancy.
• Easier debugging: smaller methods make it simpler to pinpoint issues, as the scope of each method is limited.

3. Large classes

Large classes have many responsibilities and over a thousand lines of code. They can be a source of decreased maintainability, reduced readability, and hindered collaboration among developers. This review aims to break large classes into smaller, more manageable units.

• Enhanced code organization: smaller classes reduce cognitive load and make it easier to navigate the codebase.
• Better maintainability: breaking down large classes simplifies maintenance and mitigates the risk of introducing errors.
• Promotes reusability: smaller, focused classes can be reused across different parts of the codebase.
• Facilitates collaboration: smaller classes make it easier for multiple developers to work on different components concurrently.

4. Long parameter list

This can be a sign that the method does too much. Long parameter lists lead to reduced readability, increased complexity, and a higher likelihood of introducing bugs. This review aims to streamline methods by reducing the number of parameters they accept.

• Improved code readability: simplified parameter lists make code more self-explanatory, reducing the need for extensive comments.
• Enhanced maintainability: refactoring long parameter lists simplifies maintenance and reduces the risk of errors during changes.
• Encourages best practices: promotes adherence to the Single Responsibility Principle.
• Facilitates code reuse: reducing parameter lists promotes the reuse of methods in different contexts.

5. Divergent change

A code smell that happens when you change the same code for different reasons. It is often a sign of code that lacks cohesion and is difficult to maintain. This review aims to restructure code so that each module or class adheres to the Single Responsibility Principle.

• Enhanced code cohesion: each module or class is focused on a single responsibility.
• Improved readability: code that adheres to the Single Responsibility Principle is easier to read, understand, and maintain.
• Easier debugging: clearer separation of responsibilities makes it simpler to pinpoint and resolve issues.
• Promotes scalability: code structured around single responsibilities is more adaptable to changes.

6. Shotgun surgery

Occurs when you need to make changes in different parts of the codebase to implement a feature or fix a bug. A single code change necessitates multiple modifications across the codebase, leading to increased complexity, higher maintenance costs, and a higher risk of errors. This review aims to refactor code to minimize the ripple effect of changes.

• Reduced code coupling: refactoring reduces coupling and promotes modularity.
• Lower maintenance costs: code that is less prone to shotgun surgery requires less effort and time for maintenance.
• Easier debugging: isolating responsibilities makes it simpler to identify the source of issues.
• Improved code quality: better code organization and fewer dependencies lead to improved overall quality.

7. Feature envy

Code that accesses data or methods from another class more often than its own. Feature envy occurs when one class or module is excessively interested in the internal details of another, leading to decreased cohesion and increased complexity. This review aims to ensure responsibilities are appropriately distributed among classes.

• Enhanced code cohesion: each class or module has well-defined responsibilities.
• Improved code readability: code that adheres to proper encapsulation is easier to understand and maintain.
• Easier debugging: clear separation of responsibilities makes it simpler to resolve issues.
• Promotes scalability: code structured around appropriate responsibilities is more adaptable to changes.

8. Data clumps

Occurs when three or more data items always appear together in the code. Data clumps are repeated groups of closely related data fields or parameters that often appear together, indicating a potential design issue. This review aims to remove data clumps by introducing more structured data representations.

• Enhanced code organization: structured data representations improve maintainability and readability.
• Reduced redundancy: encapsulating related data reduces the likelihood of inconsistent updates.
• Simplified maintenance: changes to data structures are centralized.
• Promotes scalability: structured data types are more adaptable to changes and facilitate future enhancements.

9. Primitive obsession

Not substituting particular primitive types with classes. Primitive obsession occurs when simple data types, such as integers or strings, are used excessively in place of more structured and meaningful objects.

• Improved code clarity: data is represented in a more meaningful and structured way.
• Reduced redundancy: structured objects reduce the likelihood of inconsistent data updates.
• Simplified maintenance: data structures are more cohesive and encapsulated.
• Promotes code reusability: structured objects encourage reuse and are more adaptable to changes.

10. Switch statements

Having the same switch statement in multiple places in the code. Switch statements can lead to code that is hard to maintain, less extensible, and more prone to errors when not used judiciously.

• Enhanced code maintainability: logic is encapsulated in more meaningful abstractions.
• Improved code readability: code that replaces switch statements is more self-explanatory.
• Reduced error-prone code: structured abstractions reduce the risk of introducing errors.
• Promotes code reusability: replacing switch statements often leads to more adaptable code.

11. Parallel inheritance hierarchies

Every time you make one subclass or implement one interface, you also need to make another subclass or create a class that implements another interface. Parallel inheritance hierarchies occur when two or more class hierarchies evolve independently, leading to duplication and increased complexity.

• Enhanced code maintainability: code is more organized and reduces redundancy.
• Improved code readability: eliminating parallel hierarchies makes the code easier to understand.
• Reduced code duplication: the refactoring process reduces duplication, leading to a more efficient codebase.
• Promotes code reusability: changes are made in a centralized manner.

12. Lazy class

A lazy class is simply a class with one or two methods that you could move to another class. Lazy classes can lead to code bloat, reduced maintainability, and increased complexity. This review aims to refactor or remove these underutilized components.

• Leaner codebase: removing or refactoring lazy classes reduces bloat and complexity.
• Improved readability: code that eliminates irrelevant classes is more straightforward.
• Simplified maintenance: a smaller, focused codebase is easier to maintain and extend.
• Promotes code clarity: helps developers focus on essential functionality.

13. Speculative generality

Code that is over-engineered. Speculative generality occurs when code includes unnecessary abstractions, design patterns, or features that are not currently required and might never be. This review aims to eliminate such unnecessary complexities.

• Simpler codebase: removing speculative generality makes the code more efficient and easier to maintain.
• Improved readability: code without unnecessary complexity is more straightforward.
• Simplified maintenance: a leaner codebase reduces the risk of introducing errors.
• Efficiency gains: reducing unnecessary abstractions and features can lead to performance improvements.

14. Temporary field

Adds complexity by unnecessarily storing a result into a variable, instead of returning the result itself. Temporary fields are variables or properties used for a short duration, often within a limited scope, that may not serve a long-term purpose.

• Simplified codebase: removing or better managing temporary fields reduces complexity and redundancy.
• Improved code readability: code that eliminates unnecessary fields is more straightforward.
• Reduced risk of errors: eliminating temporary fields reduces the risk of introducing errors.
• Promotes code clarity: properly managing data improves clarity and simplifies troubleshooting.

15. Message chains

A class that calls an object which calls another object: customer.getLastInvoice().getOrder().getOrderId(). Message chains occur when one object sends a chain of method calls to another object, often resulting in tight coupling and reduced maintainability.

• Enhanced code cohesion: behavior is encapsulated more appropriately.
• Improved code readability: eliminating complex message chains makes code more straightforward.
• Reduced code coupling: proper management reduces tight coupling between objects.
• Promotes code maintainability: code that avoids excessive message chains is easier to maintain.

16. Middle man

A class that only delegates calls to other classes. Middle man code occurs when an intermediary class or component simply delegates calls to another, providing little or no additional functionality.

• Simplified codebase: removing or optimizing middle man components reduces redundancy.
• Improved code readability: eliminating unnecessary intermediaries is more straightforward.
• Reduced code redundancy: proper management leads to a more efficient codebase.
• Promotes code clarity: optimizing middle men helps developers focus on essential functionality.

17. Inappropriate intimacy

This can happen when classes are tightly coupled, meaning they rely on each other too much. Excessive coupling or dependencies between classes that share too much information make the code less maintainable, less modular, and more error-prone.

• Enhanced code modularity: more modular and encapsulated code is easier to maintain and extend.
• Reduced code coupling: proper dependency management makes the codebase more adaptable.
• Improved code readability: eliminating inappropriate intimacy makes code more straightforward.
• Promotes code reusability: decoupling components often leads to more reusable code.

18. Alternative classes with different interfaces

Classes that have different method signatures but almost the same behavior. This can lead to confusion, maintenance challenges, and code inconsistencies. This review aims to align the interfaces of alternative classes or provide clearer naming and documentation.

• Enhanced code consistency: aligning interfaces and naming conventions reduces confusion.
• Improved code readability: code that addresses differing interfaces is easier to understand.
• Reduced risk of errors: consistent interfaces reduce the risk of introducing errors.
• Promotes code reusability: aligning interfaces often leads to more reusable code.

19. Refused request

Happens when subclasses don't use methods and fields inherited from the parent class. This causes inefficiencies and potential issues in the codebase. This review focuses on ensuring that the class or component appropriately handles all expected requests.

• Enhanced code robustness: the class or component can effectively serve clients' needs.
• Improved client interactions: addressing refused requests reduces frustration and misunderstandings.
• Reduced risk of errors: improved error handling reduces the risk of unexpected issues.
• Enhanced software reliability: robust handling of client requests contributes to overall reliability.

20. Comments

Comments play a vital role in enhancing code understanding, maintainability, and collaboration, but they are not overused. This review aims to ensure that code comments are clear, concise, and provide valuable insights to developers.

• Enhanced code readability: clear, concise, and relevant comments improve code understanding.
• Improved code maintenance: well-documented code reduces the time required for maintenance and troubleshooting.
• Effective collaboration: high-quality comments facilitate knowledge sharing and best practices.
• Legal compliance: accurate license and copyright notices within comments help ensure legal compliance.

21. Dead code

Code that is no longer used. Dead code serves no purpose and adds unnecessary complexity, leading to maintenance challenges and an increased risk of errors. This review aims to refactor or remove such code.

• Simplified codebase: removing dead code reduces complexity, making it easier to understand and maintain.
• Improved code readability: a codebase free of unused code is more straightforward to work with.
• Reduced risk of errors: eliminating dead code reduces the risk of introducing errors.
• Performance gains: dead code removal can lead to performance improvements, as unnecessary computations are eliminated.