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:
Static Class GlobalMethods
GenerateNewCarId() : Int32 // generates a new Car ID
GenerateNewCustomerId() : Int32 // generates a new Customer ID
VerifyEmail(email) : Boolean // verifies a customer email address using RegEx
Age(dateOfBirth) : Int32 // returns a Customers Age
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 Vehicle
NumberOfWheels : Int32
Doors : Int32
Color : ColorEnum
Weight : Int32
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:
Public Class Vehicle
ToHtml() : String
NumberOfWheels : Int32
Doors : Int32
Color : ColorEnum
Weight : Int32
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 Inteface IVehicle
ToHtml() : String
NumberOfWheels : Int32
Doors : Int32
Color : ColorEnum
Weight : Int32
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.