On Readability and Maintainability

In the last 6 weeks, I have been spending a lot of my time rearchitecting my software. Along with this comes a lot of refactoring. It gets me to thinking about readability and maintainability. I came across a situation where there were several methods with the same boilerplate code. I reduced this down to a single method that takes a delegate and each dependant method calls into the single method with an implementation of a filter. When it was all said and done, I had sacrificed readability for maintainability. Following is a contrived example that shows my dilema:

Before:

public ICollection<string> GetDataWithABC(IDataSource dataSource)
{
    List<string> result = new List<string>();

    using(IDataReader reader = dataSource.GetData("ABC"))
    {
        while(reader.Read())
        {
            string value = reader.GetString(0);
            if(CheckAboutABC(value) && value.Contains("JUNK"))
                result.Add(value);
        }
    }

    return result;
}

public ICollection<string> GetDataWithDEF(IDataSource dataSource)
{
    List<string> result = new List<string>();

    using (IDataReader reader = dataSource.GetData("DEF"))
    {
        while (reader.Read())
        {
            string value = reader.GetString(0);
            if (value.Contains("BLAH"))
                result.Add(value);
        }
    }

    return result;
}

public ICollection<string> GetDataWithXYZ(IDataSource dataSource)
{
    List<string> result = new List<string>();

    using (IDataReader reader = dataSource.GetData("XYZ"))
    {
        while (reader.Read())
        {
            string value = reader.GetString(0);
            if (CheckAboutXYZ(value) && CheckSomethingElseXYZ(value))
                result.Add(value);
        }
    }

    return result;
}

Notice how each of these methods has the same boilerplate code. They are already pretty straight-forward:

1. Create a list to return
2. Get the data reader from the data source
3. While there is data, Get the data, filter it and add the values to the list
4. Return the list

I think this code is pretty readable. Of course, it is not maintainable. If IDataSource is changed, for example, to return an IEnumerable, then we have to change all three methods. In my example, there were more like 10 methods with the same boilerplate code.

The only real difference in any of these methods is the filter. Why not extract the boilerplate code into a single method and pass a delegate to do the filtering? The result looks like this:

After:

private delegate bool DataFilter(string value);
private static ICollection<string> GetDataWithFilter(IDataSource dataSource, DataFilter filter)
{
    List result<string> = new List<string>();

    using (IDataReader reader = dataSource.GetData("ABC"))
    {
        while (reader.Read())
        {
            string value = reader.GetString(0);
            if (filter(value))
                result.Add(value);
        }
    }

    return result;
}

public ICollection<string> GetDataWithABC(IDataSource dataSource)
{
    return GetDataWithFilter(dataSource, delegate(string value)
    {
        return CheckAboutABC(value) && value.Contains("JUNK");
    });
}

public ICollection<string> GetDataWithDEF(IDataSource dataSource)
{
    return GetDataWithFilter(dataSource, delegate(string value)
    {
        return value.Contains("BLAH");
    });
}

public ICollection<string> GetDataWithXYZ(IDataSource dataSource)
{
    return GetDataWithFilter(dataSource, delegate(string value)
    {
        return CheckAboutXYZ(value) && CheckSomethingElseXYZ(value);
    });
}

I love this approach. There is no duplication of code anymore. If something changes with the boilerplate code, it can be changed in one spot. I have saved lines of code and everything is more compact.

Unfortunately, I think that the code has become less readable. First, it becomes difficult to discern what is going on there at first glance. The use of the inline delegate is a bit confusing and can trip up people who read this for the first time. Second, the double return is confusing. The inner return returns a bool where the method returns an ICollection. I have seen this trip up experienced developers who glance at the method. It is also more difficult to debug.

I could always strip the delegates out into separate methods, but I believe that is even more unreadable. Whats worse, is that we didn't really save many lines of code. Only 9 lines of code were saved. (more were saved in my real-world scenario)

So what is better? Readability or Maintainability?

A colleague whom I have a lot of respect for told me "Always go with readable". I guess, in this case, I have to disagree with him and go with Maintainability. Duplication of code, for me, is a real big no-no. Duplication 10 times is an even bigger no-no. In this case, I think the maintainability gain outweighs the readability loss. I will even argue that as you shorten the length of each individual method, the aggregated file becomes more readable as a whole.

As a side note, it turns out that in C# 3.0, you can use lambda functions to make it a bit more readable. Unfortunately, we aren't using 3.0 yet:

public ICollection<string> GetDataWithABC(IDataSource dataSource)
{
    return GetDataWithFilter(dataSource, value => CheckAboutABC(value) && value.Contains("JUNK"));
}