What to look for when reviewing code for SOLID principle ‘violations’

If your tasked with doing a code review of a fellow developers code or indeed are looking to improve an existing code base as a whole, one good set of principles to review the code by is the SOLID principles by Uncle Bob.

There are five SOLID principles in total and these courtesy of Wikipedia are:


Here’s what you should keep an eye out for if looking for violations of these principles:

Single responsibility principle – look for large classes which contain lots of functionality, chances are those classes could be broken up into smaller more focused ones. When your dealing with already small size classes the line is a little less clear as really what is the definition of ‘single’.

Open/closed principle – look for long blocks of if/else statements which check an object’s type and then does some similar action X (but differently) based on the result. When you add a new relevant type the class with this if/else type check code has to be modified to accommodate the new type, whereas the better way would be to use polymorphism either through interfaces or abstract classes.

Liskov substitution principle – this one may be a little tougher to spot and indeed is it often the toughest principle for developers to grasp. Look for subclass methods which change the behaviour (even if only sometimes) of a base class method in such a way that consumers can’t call the derived method and have it ALWAYS behave as if it was the base class method. To find LSP violations look for things like:

Subclasses with overridden methods which throw NotImplementedException().

Subclass methods which enforce stricter rules on parameters than their base.  For example… a base method accepts an integer as parameter but overridden method throws exception if this integer is not positive.

Methods which appear from a signature point of view to operate on a base class (or interface) but then within the method some type checking occurs. 

Interface segregation principle – look for large (fat) interfaces with a lot of unfocused methods, also look at interfaces whose clients throw NotImplementedException() a lot.

Dependency inversion principle – look for a lot of new statements.  New statements means concrete instantiations, which means coupling. Better to use a dependency injection framework or even (if you must) poor man’s dependency injection and push the creation of your objects up higher in the stack while simply passing down interfaces which provide a functionality contract but which makes no assertions about how that functionality is implemented.

Related Links

Overview of the five different principles. I always seem to be reading this article come interview time.

Register Castle Windsor IOC components via convention

Favouring convention over configuration means less change points (and remember points) for developers to deal with when new code needs to be added to software. My team tries to follow this guideline as much as possible but given the fact we are new users of Castle Windsor IOC, we were not aware that we could by convention have an IXXXService implemented by the first found XXXService.

We had code like the following in our IOC bootstrapper file:


which meant of course we had to explicity update the bootstrapper file if we created a new service. A new colleague started the other day and refactored to this:


Now we only have to explicity register exceptions to the interface IXYZ being implemented by class XYZ rule. Pretty neat I thought.

We still use Castle Windsor 2, for Castle Windsor 3 I believe you need to use