All in the Name of DRY

A while back, I was doing a code review. At some point I encountered some constructs that I see quite often. While I was describing and building my case, I was thinking that I might as well write a blog post about it. The code that I encountered looked similar to the following:

public interface IUser
{
    Int32 Id { get; }
    String FirstName { get; }
    String LastName { get; }
    Boolean IsValid { get; }
}

public class ValidUser : IUser
{
    public Int32 Id { get; }
    public String FirstName { get; }
    public String LastName { get; }
    public virtual Boolean IsValid => true;

    public ValidUser(Int32 id, String firstName, String lastName)
    {
        Id = id;
        FirstName = firstName;
        LastName = lastName;
    }
}

public class InvalidUser : ValidUser
{
    public override Boolean IsValid => false;

    public InvalidUser(int id)
        : base(id, string.Empty, string.Empty)
    {}
}

There are several issues with this code. The first thing that I would like to mention is that I don’t like declaring properties on interfaces. But that’s not what I want to elaborate on in this blog post. The thing that really interested me was the starting premise that led the developer in question to make these design choices. And this starting premise is called the DRY principle.

DRY

The DRY principle is an acronym that stands for “Don’t Repeat Yourself”. This principle originates from the excellent book The Pragmatic Programmer written by Andrew Hunt and Dave Thomas. Around the same time, this principle has also been stated in the book Extreme Programming Explained by Kent Beck where it is described as the “Once and Only Once Rule”.

It states that:

“Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.”

Code that isn’t DRY has the same algorithms (=knowledge) expressed in two or more places. If you change one, you have to remember to change the code at the other places as well. A developer new to the team that performs this change probably doesn’t know about these other places. Therefore we say that such code is hard to change and maintain.

The DRY principle is a very important design rule that luckily has become quite popular over the years. It is said that it promotes the orthogonality of the code by isolating changes to only those parts of the system that must change.

Too DRY

Now that we know about the DRY principle and it’s benefits, let’s have another look at our code example. It seems that there’s a concept of a “valid user” and an “invalid user”. When looking at their implementation, the InvalidUser is derived from the ValidUser class. In object-oriented terms, when a class inherits from another class, this basically means that an “IS A” relationship is being established. In our example, this results in an “invalid user” also being a “valid user”, which is definitely a bit fishy to say the least.

I’m pretty sure that this code was written with the best intentions in mind. The developer in question wanted to adhere to the (too) DRY principle in order to avoid the same property definitions on both the ValidUser and the InvalidUser class. But here’s the thing.

Properties that happen to have the same name, that are defined on more than one class, do NOT qualify as code duplication!

So I wonder why some developers go to great lengths and introduce unnecessary complexity just to avoid a few property definitions. Going back to our code example, we notice that this design choice also has some indirect consequences as well.

By deriving the InvalidUser class from the ValidUser class, we basically give up type checking. There’s no way that I can write code that verifies whether a user is valid or not based on it’s type.

IUser user = ... // Retrieve an instance of a user
if(user is ValidUser)
    Console.WriteLine("Woohoo, a valid user");

The condition of the if statement will always be true, regardless of the type of user. This is probably the reason why the IsValid property has been added.

IUser user = ... // Retrieve an instance of a user
if(user.IsValid)
    Console.WriteLine("Woohoo, a valid user");

Less DRY

After performing some refactoring, the code looks a tiny bit better.

public interface IAmUser
{
    Int32 Id { get; }
    String FirstName { get; }
    String LastName { get; }
    Boolean IsValid { get; }
}

public class ValidUser : IAmUser
{
    public Int32 Id { get; }
    public String FirstName { get; }
    public String LastName { get; }
    public virtual Boolean IsValid => true;

    public ValidUser(Int32 id, String firstName, String lastName)
    {
        Id = id;
        FirstName = firstName;
        LastName = lastName;
    }

    public static IAmUser Invalidate(Int32 id)
    {
        return new InvalidUser(id);
    }
}

public class InvalidUser : IAmUser
{
    public Int32 Id { get; }
    public String FirstName { get; }
    public String LastName { get; }
    public virtual Boolean IsValid => false;

    public InvalidUser(Int32 id)
    {
        Id = id;
        FirstName = string.Empty;
        LastName = string.Empty;
    }
}

We got rid of the inheritance structure and just implemented the properties that are needed. We also added a static Invalidate method on the ValidUser class that creates an instance of the InvalidUser class.

There are several more things that can and should be improved, like the violation of the Interface Seggregation Principle (ISP) (again, I don’t like to define properties on interfaces), checking for equality by implementing the Equals/GetHashCode methods, etc. … But I hope I made my point.