Red Flags: SOLID Principle Violations Every Code Reviewer Should Catch
From Good to Great: Using SOLID Principles to Elevate Your Code Reviews
Celebrate 15h of August with 35% off on our paid subscription โ limited-time offer, donโt miss out!
Hey fellow developers! ๐ As an experienced Software Engineer and Tech lead who spends a fair bit of time peering at pull requests, I can tell you that code review isn't just about finding typos or missing semicolons.
It's our chance to bake in quality, foster shared understanding, and ultimately, build robust, scalable, and maintainable software.
And for me, nothing helps achieve that more than keeping the SOLID principles firmly in mind.
SOLID is an acronym for five fundamental principles of object-oriented design proposed by Robert C. Martin (Uncle Bob), author of Clean Code, Clean Coder, and Clean Architecture books, and one of the most respected name in the programming world.
They might sound like abstract theory, but trust me, they're incredibly practical. Let's walk through each one and see how they empower you to conduct truly effective code reviews.
By the way, I am also running a sale and you can get our paid plan for good 35% discount. Instead of paying 50$ / year, you pay 32.5$ / year (only 3$ / month)! Thatโs the price of less than one cup of decent coffee.
Here are the benefits you unlock with a paid subscription:
Get access to paid subscribers posts.
Full archive of 220+ posts on System Design, Coding, Java, AI, and LLM
Many expense it with team's learning budget
Spotting SOLID Principle Violations in Code Reviews: A Practical Guide
Now, letโs start with the real SOLID stuff ..
S: Single Responsibility Principle (SRP) - "One Reason to Change"
This one is often violated and is a common source of headaches down the line!
What it means: A class or module should have one, and only one, job. If there's more than one reason for a class to change, it's doing too much.
How to spot it in code review: When looking at a class, ask yourself:
"If the requirements for feature A change, will this class also need to change because it handles feature B?"
"Does this class have multiple concerns, like database interaction, business logic, and reporting?"
Example (Violation):
class UserProcessor {
void saveUser(User user) { /* saves to DB */ }
void validateUser(User user) { /* validates */ }
void sendWelcomeEmail(User user) { /* sends email */ }
// ... more methods
}
Here,
UserProcessor
is doing three different jobs: persistence, validation, and notification. If the email system changes, or the validation rules, this one class needs modification.Fix (Adherence): Break it down!
class UserRepository {
void save(User user) { /* saves to DB */ }
}
class UserValidator {
void validate(User user) { /* validates */ }
}
class EmailService {
void sendWelcome(User user) { /* sends email */ }
}
// A higher-level service orchestrates these:
class UserRegistrationService {
private final UserRepository userRepository;
private final UserValidator userValidator;
private final EmailService emailService;
// Constructor injection for dependencies
public UserRegistrationService(UserRepository repo, UserValidator validator, EmailService emailer) {
this.userRepository = repo;
this.userValidator = validator;
this.emailService = emailer;
}
void registerUser(User user) {
userValidator.validate(user);
userRepository.save(user);
emailService.sendWelcome(user);
}
}
Now, each class has a single reason to change, making them easier to test and maintain. โ
By the way, nowadays you can integrate code review tools like CodeRabbit to catch SOLID violations automatically. Though, I expect senior engineers to be competent enough to catch these manually, using tools are great but we should also be able to do our job without them.
O: Open/Closed Principle (OCP) - "Open for Extension, Closed for Modification"
This principle is all about making your code adaptable without constant refactoring. It also minimizes the risk because your code will be open for extension but closed for modification, which means, you wonโt be touching your tried and tested code.
What it means: You should be able to add new functionality without altering existing, working code. Think of it like adding a new app to your phone without having to rewrite the phone's operating system.
How to spot it in code review: Look for large
if-else
orswitch
statements that might need to grow every time a new variant of a feature is introduced.Example (Violation):
class InvoiceCalculator {
double calculateTotal(Invoice invoice, String customerType) {
double total = invoice.getAmount();
if ("Premium".equals(customerType)) {
total *= 0.9; // 10% discount
} else if ("VIP".equals(customerType)) {
total *= 0.8; // 20% discount
}
// What if a new 'Loyalty' customer type is added? We modify this class.
return total;
}
}
Adding a new customer type means modifying
InvoiceCalculator
.Fix (Adherence): Use polymorphism with interfaces or abstract classes.
interface DiscountStrategy {
double applyDiscount(double amount);
}
class NoDiscount implements DiscountStrategy {
public double applyDiscount(double amount) { return amount; }
}
class PremiumDiscount implements DiscountStrategy {
public double applyDiscount(double amount) { return amount * 0.9; }
}
class VIPDiscount implements DiscountStrategy {
public double applyDiscount(double amount) { return amount * 0.8; }
}
class InvoiceCalculator {
double calculateTotal(Invoice invoice, DiscountStrategy strategy) {
return strategy.applyDiscount(invoice.getAmount());
}
}
Now, to add a new discount, you just create a new
DiscountStrategy
implementation โ no modification toInvoiceCalculator
! โ
By the way, this is the 6th article in my writing better code series, if you havenโt read previous articles, I highly recommend to read them to learn small tips which can have big impacts if applies regularly
Here is list of all the tips I have shared before:
L: Liskov Substitution Principle (LSP) - "Subtypes Must Be Substitutable for Their Base Types"
This one is crucial for maintaining sensible inheritance hierarchies.
What it means: If you have a class
A
and a classB
inherits fromA
, you should be able to use an object ofB
anywhere an object ofA
is expected, without breaking the program or producing unexpected behavior.How to spot it in code review: Look for subclasses that throw unexpected exceptions, return
null
when the superclass promises a value, or drastically alter the behavior expected from the superclass's contract.Example (Violation):
class Bird {
void fly() { /* default flying behavior */ }
}
class Ostrich extends Bird {
@Override
void fly() {
throw new UnsupportedOperationException("Ostriches cannot fly!");
}
}
// Client code
void makeBirdFly(Bird bird) {
bird.fly(); // This will crash if 'bird' is an Ostrich
}
The
Ostrich
breaks theBird
's contract. ABird
is expected to fly, but anOstrich
doesn't.Fix (Adherence): Rethink your hierarchy or introduce interfaces.
interface Flyable {
void fly();
}
class Bird { /* general bird properties */ }
class Sparrow extends Bird implements Flyable {
public void fly() { /* flying behavior */ }
}
class Ostrich extends Bird {
// No fly() method, as it's not Flyable
}
// Client code
void makeFlyableBirdFly(Flyable bird) {
bird.fly(); // Now, we're sure it can fly!
}
By using the
Flyable
interface, you only expect flying behavior from objects that can truly fly, making your system more robust. โ
I: Interface Segregation Principle (ISP) - "Clients Should Not Be Forced to Depend on Interfaces They Do Not Use"
This prevents "fat" interfaces from creating unnecessary dependencies.
What it means: Instead of one large interface, create smaller, more specific interfaces tailored to what individual clients need.
How to spot it in code review: Look for an interface with many methods, where its implementers often have empty method bodies, throw
UnsupportedOperationException
, or comment out sections because they don't use all the methods.Example (Violation):
interface Worker {
void doWork();
void eatLunch();
void takeVacation();
void rechargeBattery(); // Only robots need this
}
class HumanWorker implements Worker {
public void doWork() { /* ... */ }
public void eatLunch() { /* ... */ }
public void takeVacation() { /* ... */ }
public void rechargeBattery() { /* Does nothing for humans */ } // Violation!
}
```HumanWorker` is forced to implement `rechargeBattery()` which is irrelevant.
Fix (Adherence): Segment the interface.
interface Workable {
void doWork();
}
interface LunchBreakCapable {
void eatLunch();
}
interface VacationCapable {
void takeVacation();
}
interface BatteryPowered {
void rechargeBattery();
}
class HumanWorker implements Workable, LunchBreakCapable, VacationCapable { /* ... */ }
class RobotWorker implements Workable, BatteryPowered { /* ... */ }
Now, each worker only implements the interfaces relevant to its capabilities, reducing unnecessary dependencies and cleaner code. โ
Here is a nice diagram from ByteByteGo explaining this concept visually
D: Dependency Inversion Principle (DIP) - "Depend on Abstractions, Not Concretions"
This is the backbone of highly testable and flexible codebases, often seen with Dependency Injection (DI) frameworks.
What it means:
High-level modules (business logic) shouldn't depend on low-level modules (implementation details). Both should depend on abstractions (interfaces).
Abstractions should not depend on details; details should depend on abstractions.
How to spot it in code review: Look for high-level classes directly creating instances of concrete low-level classes using
new()
. This creates tight coupling.Example (Violation):
class OrderProcessor {
private DatabaseConnector connector = new DatabaseConnector(); // Direct dependency on concrete class
void processOrder(Order order) {
connector.save(order); // Uses concrete DB logic
// ...
}
}
```OrderProcessor` (high-level) directly depends on `DatabaseConnector` (low-level concrete implementation). Changing the database type requires modifying `OrderProcessor`.
Fix (Adherence): Introduce an interface and inject the dependency.
interface OrderRepository {
void save(Order order);
}
class DatabaseOrderRepository implements OrderRepository {
// Concrete implementation using a database
public void save(Order order) { /* saves to DB */ }
}
class OrderProcessor {
private final OrderRepository orderRepository; // Depends on abstraction
// Dependency is injected via constructor
public OrderProcessor(OrderRepository repo) {
this.orderRepository = repo;
}
void processOrder(Order order) {
orderRepository.save(order); // Uses abstraction
// ...
}
}
Now,
OrderProcessor
doesn't care how the order is saved, only that it can be saved via theOrderRepository
interface.
You can swapDatabaseOrderRepository
with aInMemoryOrderRepository
(for testing) orNoSQLOrderRepository
without changingOrderProcessor
. โ
Wrapping Up: SOLID in Your Code Reviews
Thatโs all in this post guys. Applying SOLID principles during code reviews is more than just a checklist; it's a mindset shift. It transforms reviews from simple bug-spotting into opportunities for architectural refinement.
By consistently asking the right questions through the lens of SOLID, you're not just approving code; you're actively contributing to building a more resilient, adaptable, and understandable software system.
What's more, AI-powered tools like CodeRabbit are increasingly becoming valuable allies in this process. They can automatically analyze pull requests, identifying potential SOLID violations, code smells, and suggesting improvements, which frees up human reviewers to focus on higher-level architectural decisions and business logic.
It helps teams speak a common language about design quality and makes future development a much smoother ride.
What's your go-to SOLID principle to check during a review? And which one do you find most often violated in code you review?
Share your thoughts below!
Celebrate 15h of August with 35% off on our paid subscription โ limited-time offer, donโt miss out!
Other AI, System Design, and Clean Code Articles you may like