This post originated from an RSS feed registered with .NET Buzz by Raymond Lewallen. Original Post: Learn from my mistakes - refactoring and fixing design smells using common patterns Feed Title: Raymond Lewallen Feed URL: /error.htm?aspxerrorpath=/blogs/raymond.lewallen/rss.aspx Feed Description: Patterns and Practices, OOP, .Net and Sql Latest .NET Buzz Posts Latest .NET Buzz Posts by Raymond Lewallen Latest Posts From Raymond Lewallen

So I’m working in a maintenance release right now.  Very little added functionality.  My current release at work is primarily to adjust to domain logic changes.  This starts by adjusting the unit tests to accomodate the changed logic, and then fixing the code to fix the tests.  Still, due to the nature of the changes, a lot of regression testing will still be involved.

This release also gives me an opportunity to go back and fix some serious design errors that I have lazily let make their way into the code.  I want to cover a few these now, tell you why they are design mistakes, and how to fix them.  Feel free to leave nasty comments here and add me to your wall of coding shame for some of these.  For the most part, I was being lazy, but recognizing these mistakes and fixing them as soon as I have the opportunity should award me back some brownie points, right?

Poor use of some static classes

Static classes are something that you usually don’t find too often in a framework, and rightly so.  They are there to only support the instance classes of the rest of your framework, but there is a right way and wrong way to do that.  Of the several static classes I have, the majority are designed correctly, but two of them are not.

Here’s the design mistake: I have 2 static classes that are a catch-all for miscellaneous methods that support the framework.  Let’s say you have an application for tracking car sales.  To mimic the design mistake, I have a static class such as the following:

Now, each of these are fine being static methods, but they are for the most part, unrelated.  Better organization needs to occur here.  I should create a new static class in the namespace for the Cars and another in the namespace for the Customers, and move each Id method to its corresponding static class so its coupled with the rest of its entities.  The Age function can be moved to the Customer instance class itself, where the dateOfBirth is already stored as part of the state of the object and we can remove the parameter from the function.  VerifyEmail can perhaps be made a private method inside the Customer instance class, and be called when we load the object state, or simply {get} the Email property.  How to handle the logic if the VerifyEmail fails is dependant upon your particular situation.  My actual code isn’t this blatently bad, but you get the idea.

Reducing If/Then and Switch logic

I could talk about this, but Jeremy has an excellent post on using the Strategy Pattern, which is I need to implement into some code that has become bloated and overly complex.

These next few pieces of design problems are all coupled together to reach a common goal.

Unify Interfaces

The idea around unifying classes (the actual refactoring applied here is referred to as Unify interface) revolves around evolving subclasses without evolving your interfaces or superclass.  As my application has evolved, so has a number of classes that either implement an interface or inherit a superclass.  The added methods to the subclasses don’t exist in the superclass, such as the following:

Public Class Car : Vehicle
     ToHtml() : String
     NumberOfWheels : Int32
     Doors : Int32
     Color : ColorEnum
     Weight : Int32

As you can see, my subclass contains a method, ToHtml, that returns a description value to display on a webpage, that doesn’t exist in the superclass.  To unify the interfaces of both, I need to copy the missing methods to the superclass.  Now my superclass Vehicle looks like this:

Now my subclass and my superclass share a common set of functionality, i.e. a common interface.

Extract Interfaces

Now that this is done, I’m ready for another refactoring to take place: Extract Interface.  So now I’m left with the following:

Public Class Vehicle : IVehicle
     ToHtml() : String
     NumberOfWheels : Int32
     Doors : Int32
     Color : ColorEnum
     Weight : Int32

Public Class Car : Vehicle
     ToHtml() : String
     NumberOfWheels : Int32
     Doors : Int32
     Color : ColorEnum
     Weight : Int32

Now I’m much closer to a better design than what I started with for this set of classes.

Make unit testing easier

To take it one step further, imagine a class that relies on the Car object to do its work.  And let’s also say we have a unit test on this class, but in order for the test to work, we have to have a connection to the database.  Well, we don’t like that very much, so a good way to remove that database layer is to use dependency injection.  Take the following class:

class Ticket

    {

        private Car _car;

        private Int32 _carId;

        public Ticket() {}

        public Ticket(Int32 carId)

        {

            _carId = carId;

        }

        public void Load()

        {

            // do something to load the car

            // object using the carid, if it

            // is > 0

        }

    }

In order to test this class and its methods (that I didn't bother adding for the examples, but there are other methods on the class we need to test), we must have a Car object, but the only way we can get it right now is to load one from the database after calling the constuctor that takes a carId parameter.  Not very good.  Big design mistake on my part, and I have plenty of places where this needs to be fixed in my code.  Take time to curse me under your breathe right now.

How do we fix it?  If you read the above link, and you should have, you know the answer.  We implement dependency injection into the equation.  We are going to use a mock Car object, give that object to the Ticket class, and use that mock object to do our unit testing, rather than requiring a connection to the database.  When we’re done, and again, you know how all this ties in because you read the above link, we end up with this:

class Ticket

    {

        private IVehicle _car;

        private Int32 _carId;

        public Ticket() {}

        public Ticket(Int32 carId)

        {

            _carId = carId;

        }

        public Ticket(IVehicle car)

        {

            _car = car;

        }

        public void Load()

        {

            // do something to load the car

            // object using the carid, if it

            // is > 0

        }

    }

Now we can just give the Ticket class a Car to use to do its testing.  This wouldn’t have been possible if I hadn’t gone through the above Unify Interfaces and Extract Interface refactorings first.

So there are the major design smells that I’m going to be fixing over the next two months.  I hope you read this, learn from my design laziness and mistakes so that you don’t have to have a maintenance release.  The people signing your paycheck really, really hate maintenance releases, because they are paying you for ZERO or minimal functionality for that release.  Its best to avoid these mistakes altogether, but incase you need to sneak in a few similar fixes, hopefully my above roadmap will help you out.