What to look for in a Code Review: SOLID Principles

Trisha Gee

This is part 5 of 6 posts on what to look for in a code review. See other posts from the series.

In today’s post we’ll look more closely at the design of the code itself, specifically checking to see if it follows good practice Object Oriented Design.  As with all the other areas we’ve covered, not all teams will prioritise this as the highest value area to check, but if you are trying to follow SOLID Principles, or trying to move your code in that direction, here are some pointers that might help.

What is SOLID?

The SOLID Principles are five core principles of Object Oriented design and programming. The purpose of this post is not to educate you on what these principles are or go into depth about why you might follow them, but instead to point those performing code reviews to code smells that might be a result of not following these principles.

SOLID stands for:

• S – Single Responsibility Principle
• O – Open-Closed Principle
• L – Liskov Substitution Principle
• I – Interface Segregation Principle
• D – Dependency Inversion Principle

Single Responsibility Principle (SRP)

There should never be more than one reason for a class to change.

This can sometimes be hard to spot from a single code review. By definition, the author is (or should be) applying a single reason to change the code base – a bug fix, a new feature, a focussed refactoring.

You want to look at which methods in a class are likely to change at the same time, and which clusters of methods are unlikely to ever be changed by a change to the other methods. For example:

This side-by-side diff from Upsource shows that a new piece of functionality has been added to TweetMonitor, the ability to draw the top ten Tweeters in a leaderboard on some sort of user interface. While this seems reasonable because it uses the data being gathered by the onMessage method, there are indications that this violates SRP. The onMessage and getTweetMessageFromFullTweet methods are both about receiving and parsing a Twitter message, whereas draw is all about reorganising that data for displaying on a UI.

The reviewer should flag these two responsibilities, and then work out with the author a better way of separating these features: perhaps by moving the Twitter string parsing into a different class; or by creating a different class that’s responsible for rendering the leaderboard.

Open-Closed Principle (OCP)

Software entities should be open for extension, but closed for modification.

As a reviewer, you might see indications that this principle is being violated if you see a series of if statements checking for things of a particular type:

If you were reviewing the code above, it should be clear to you that when a new Event type is added into the system, the creator of the new event type is probably going to have to add another else to this method to deal with the new event type.

It would be better to use polymorphism to remove this if:

As always, there’s more than one solution to this problem, but the key will be removing the complex if/else and the instanceof checks.

Liskov Substitution Principle (LSP)

Functions that use references to base classes must be able to use objects of derived classes without knowing it.

One easy way to spot violations of this principle is to look for explicit casting. If you have to cast a object to some type, you are not using the base class without knowledge of the derived classes.

More subtle violations can be found when checking:

• Preconditions cannot be strengthened in a subtype.
• Postconditions cannot be weakened in a subtype.

Imagine, for example, we have an abstract Order with a number of subclasses – BookOrder, ElectronicsOrder and so on. The placeOrder method could take a Warehouse, and could use this to change the inventory levels of the physical items in the warehouse:

Now imagine we introduce the idea of electronic gift cards, which simply add balance to a wallet but do not require physical inventory. If implemented as a GiftCardOrder, the placeOrder method would not have to use the warehouse parameter:

This might seem like a logical use of inheritance, but in fact you could argue that code that uses GiftCardOrder could expect similar behaviour from this class as the other classes, i.e. you could expect this to pass for all subtypes:

But this will not pass, as GiftCardOrders have a different type of order behaviour. If you’re reviewing this sort of code, question the use of inheritance here – maybe the order behaviour can be plugged in using composition instead of inheritance.

Interface Segregation Principle (ISP)

Many client specific interfaces are better than one general purpose interface.

Some code that violates this principle will be easy to identify due to having interfaces with a lot of methods on.  This principle compliments SRP, as you may see that an interface with many methods is actually responsible for more than one area of functionality.

But sometimes even an interface with just two methods could be split into two interfaces:

In this example, given that there are times when the decode method might not be needed, and also that a codec can probably be treated as either an encoder or a decoder depending upon where it’s used, it may be better to split the SimpleCodec interface into an Encoder and a Decoder. Some classes may choose to implement both, but it will not be necessary for implementations to override methods they do not need, or for classes that only need an Encoder to be aware that their Encoder instance also implements decode.

Dependency Inversion Principle (DIP)

Depend upon Abstractions. Do not depend upon concretions.

While it may be tempting to look for simple cases that violate this, like liberal use of the new keyword (instead of using Dependency Injection or factories, for example) and overfamiliarity with your collection types (e.g. declaring ArrayList variables or parameters instead of List), as a reviewer you should be looking to make sure the code author has used or created the correct abstractions in the code under review.

For example, service-level code that uses a direct connection to a database to read and write data:

This code is dependent on a lot of specific implementation details: JDBC as a connection to a (relational) database; database-specific SQL; knowledge of the database structure; and so on. This does belong somewhere in your system, but not here where there are other methods that don’t need to know about databases. Better to extract a DAO or use the Repository pattern, and inject the DAO or repository into this service.

Summary

Some code smells that might indicate one or more of the SOLID Principles have been violated:

• Long if/else statements
• Casting to a subtype
• Many public methods
• Implemented methods that throw UnsupportedOperationException

As with all design questions, finding a balance between following these principles and knowingly bending the rules is down to your team’s preferences. But if you see complex code in a code review, you might find that applying one of these principles will provide a simpler, more understandable, solution.