Tuesday, April 3, 2012

Favoring Inheritance

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.

Thursday, March 22, 2012

Abstraction by methods..

I recently wanted to refactor a database connectivity framework to support another type of key in the system.

At first glance, this seemed easy.  Every object implements an interface called IPathWithObjectID, that specifies the object must have a property:
object  ObjectID { get; }

Since ObjectID can be any object, it should be trivial to switch things to a string or something else.

The problem, however, lies in the details.

Most methods did this:

int ID = (int) MyObject.ObjectID;

At the time it seemed simple enough, but in hindsight, this is very limited.

A better approach would be to encapsulate the type cast into a method, which is what I refactored the code to do:

public int GetObjectKeyAsInt(IComparable ObjectKey)
{
    if (ObjectKey != null)
    {
        if (ObjectKey is int)
            return (int)ObjectKey;

        throw new ArgumentException("The ObjectKey property is expected to be of type 'int'.");
    }

    throw new ArgumentException("The ObjectKey is null!");
}

Now, in reality all this method does is do a type cast, with some checks. But the power in it is later on I can refactor what is stored in the ObjectID.

So, my code goes from:

int ID = (int) MyObject.ObjectID;

To

int ID = GetObjectKeyAsInt(MyObject.ObjectID);

Maybe I want an ObjectID to be a new class called MyDataKey. Rather than changing hundreds of different places across the code base, I just change one. The GetObjectKeyAsInt method.

Methods provide a powerful mechanism of abstraction. Use them as often as possible, even if the action seems as trivial as a typecast.