I started refactoring a core library used in software we've been developing for the past few years. One of the common tasks in the library is converting an object to a database primary key value, which is always an int.
The code was littered with the following checks at the top of every method. Of course it changes based upon the type, but the logic was the same. And worse, since I wrote the same check over and over again, I inconsistently implemented my logic checks. The general idea is to check that object is a provided type, check that the provider is a specific instance type, then get the ID:
if (patient is PathProvidedPatient == false)
throw new ArgumentException("Patient argument must be of type PathProvidedPatient");
if (((PathProvidedPatient)patient).IPatientProvider is WCF_PatientProvider == false)
throw new ArgumentException("Patient argument property IPatientProvider must be a WCF_PatientProvider class.");
WCF_PatientProvider patientProvider = ((PathProvidedPatient)patient).IPatientProvider as WCF_PatientProvider;
int PatientID = patientProvider.GetPermanentObjectKey(patientProvider.GetObjectKey((PathProvidedPatient)patient));
My first step at refactoring this code was to pull this repeated code out into methods:
public int GetPatientId(PathPatient patient)
public int GetCaseId(PathCase Case)
public int GetSampleId(PathSample Sample)
Etc...
This cleaned up the code quite a bit! But suddenly I was hit with another problem. While these individual functions knew the type, I had refactored earlier and created some generic functions to take a IPathWithObjectID, from which all the "provided" versions of the objects derive. Now I was in trouble!
A little background. Using a classic
Bridge Pattern, a provided implementation stores a reference to an ObjectID and a provider class. When a request comes into the provided object, it turns it around and calls the provider class with the objectID to fulfill the request.
And now back to the story at hand... my first go around looked something like this:
if (myObject is PathSample)
return GetSampleId((PathSample)myObject);
if (myObject is PathPatient)
return GetPatientId((PathPatient)myObject);
if (myObject is PathCase)
return GetCaseId((PathCase)myObject);
Uh oh! This isn't looking very good. Multiple if statements or a switch statement usually is a sign of a poor design.
What is needed here is abstraction. What really should happen is the provider should know how to get the database ID value. But how do we get the provider?
The first thing I looked at was provider hierarchy.
All WCF_XXXProvider classes implement their corresponding IXXXProvider interface that implements a common interface IPathProvider.
So far so good, every provider is deriving from a common type of IPathProvider.
I next looked at the Provided Objects inheritance hierarchy.
PathProvidedXXX classes all implement the IPathProvidedObject interface that implements the IPathWithObjectID interface.
Again, looks good!
I decided to add a property to the IPathProvidedObject interface:
IPathProvider Provider { get; }Now, I had to go implement that property in each of the derived classes. A pain, but no challenges because every provided class already has a property that is deriving from IPathProvider.
OK, so now regardless of the provided object, I can retrieve the provider via my new property and the ObjectID (via a property required by the IPathWithObjectID interface). So far, so good.
Next I turned to looking at my specific provider implementations. All of my providers implement the WCF_IProvider interface as well as the IXXXProvider interface. This is good as well.
I added another method to the WCF_IProvider:
int GetObjectPermanentIdentifier(IPathProvidedObject obj);
And then implemented this method in each of the classes that implement the WCF_IProvider.
So my large if statement mess cleans up nicely into a very simple method:
public int GetPermanentObjectKey(IPathProvidedObject obj)
{
if (obj.Provider is WCF_IProvider)
{
return ((WCF_IProvider)obj.Provider).GetObjectPermanentIdentifier(obj);
}
return 0;
}So, what is the advantage??
First off, the code is simple. It is easy to read and understand.
Second, it is extendable. When I add a new class (which I am sure I will), I will not forget to update some massive if statement. In fact the compiler will force me to implement the correct functionality by requiring me to implement the members of the interface.
Learning to program is not the same thing as learning to be a software engineer. It takes finesse and there is an art to making good code. But once you learn it, your code will be flexible, maintainable, and easy to test.
And having a good suite of unit tests enables you to refactor and validate your changes.
