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:

image001-2

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.

CheckBoxFor values not binding when output using foreach loop

If your outputting a list of checkboxes using CheckBoxFor from within a loop and are having problems getting the checkboxes to bind back when submitting make sure you are looping with a for loop rather than a foreach loop. This is because looping through a collection with a foreach loop will output the same name for all checkboxes which will be based on the name of the iteration variable (not the particular item in the viewModel collection) in your foreach statement and the name of the bool property and thus MVC will not find anything to bind to.

image008

Handling multiple submit buttons in ASP.NET MVC action methods

You can have multiple non nested forms all posting to different action methods from different submit buttons, you can also have multiple submit buttons posting to the same form and hence action method. In an MVC action method which is posted to by multiple submit buttons, how does one tell which button was responsible for the form post? Two easy ways are outlined on stackoverflow which I’m reposting as is here just to add a bit of extra ‘commentary’. Author is highlighted in yellow.

  1. In your razor mark-up each submit button has a different name property. Since only the name of the button which caused the submit will be posted in the HTTP request header, in your action method you can check the Request.Form collection to see which submit button property name exists and take action accordingly. In this case you don’t have to change your action method signature and given that the value property of your submit buttons is insignificant to determining which button was pressed, all your submit buttons can have the same label/text if needs be.
    image003
  2. In your razor mark-up each submit button has the same property name but different value, since only the name (and significantly value) of the button which caused the submit will be posted in the HTTP request header, you can add a property to your action method signature for MVC to bind to and populate. You can then check the value of that property and take action accordingly. This is the more MVC (and perhaps elegant) way I guess as your using default model binder rather than checking form collections directly, personally however I prefer the approach above as you don’t have to change the action method signature and the button test is not based on some visible on page property which could matter in some scenarios.image007

    Related Links

    http://stackoverflow.com/questions/19650345/mvc-razor-form-with-multiple-different-submit-buttons – Stackoverflow page with a number of solutions including the two above and an AJAX based one too.

Excluding $(this) from jQuery selector

If you want to attach a click (for example) event to a set of elements but then do something with all those elements except the one clicked that can be achieved easily with the .not() function in jQuery:

Var $contentLinks = $(“.content a”);
$contentLinks.click(function() {
$contentLinks.not(this).hide();
});

Note, caching of the set outside the click function to help from a performance point of view.

Related Links

http://api.jquery.com/not/

Viewing raw HTTP post name/value pairs sent to MVC action method

When using ASP.NET MVC sometimes after a request viewModel properties you expect to be binded and populated are not. For HTTP GET action methods debugging why this might be is easy as the params are visible in your URL.  For HTTP POST requests the name/value pairs are sent in the header so they are not as easy to review.

One could use Fiddler or the Chrome/IE developer tools to look at the raw name/values pairs being posted across from the view but the Visual Studio Locals window gives you access to this information too. Just set a breakpoint in your action method and examine the current request object in the Locals window as shown below, the particular property you want to look at is Form. In the example below we can see the raw name/value pair string isSelectedCity=4&Country=United+States

 

image001 (1)