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.
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.
Overview of the five different principles. I always seem to be reading this article come interview time.