A Tale of Two Principle Violations

When I decided to write the With statement in Fluent.NET, I never imagined that I would encounter a common bug and school myself in principles that I happen to present on. But that is what occurred, and I want to share it so others can avoid the same mistakes I made.

With is a Fluent version of Add, only it returns the object or interface it is acting upon rather than void. This allows you to chain methods together in a fluid manner. Instead of calling strings.Add(“Hello”); strings.Add(“World”);, you can call strings = strings.With(“Hello”).With(“World”);. Since you should write tests first, I will post my test that I expected to work.

[TestMethod]
public void With()
{
    var strings = new[] { "This", "is", "a" } .AsEnumerable();
    strings = strings.With("test");

    Assert.IsTrue(strings.Contains("test"));
}

With is cool because it even works with arrays! In fact, the point of it was that it would work with any IEnumerable<T>. Here was my initial attempt at an implementation.

public static IEnumerable<T> With<T>(this IEnumerable<T> source, T item)
{
    var collection = source as ICollection<T> ?? source.ToList();
    
    collection.Add(item);

    return collection;
}

I cast source to an ICollection<T> to get to that more specific interface. If it doesn’t implement ICollection<T> the null coalescing operator is used and source is convert to List<T>. In subsequent calls to With, source would be a List<T> so the casting would work. This violates the Liskov Substitution Principle because I’m operating on an ICollection or converting to one no matter what. I decided that I would knowingly do so because I wanted to extend IEnumerable and optimize for a type that it could convert to itself.

This code is buggy. It compiled, but my unit test failed.

Here’s another example of the same bug.

public static void AddFail(IList<string> strings)
{
    strings.Add("Fail");
}

If you haven’t figured it out, don’t feel bad. 40 people over 2 of my sessions haven’t answered it (out loud anyway) either. I’m certain I’ve seen this failure in many methods over the years as well.

The problem is that arrays implement ICollection<T> and IList<T>. However, arrays are a fixed size. Here’s the error I received when running my test: “System.NotSupportedException: Collection was of a fixed size.”

If you’re performing operations that would modify a collection, you must check the IsReadOnly property of that collection. This fixed the bug and is what I checked into CodePlex… but don’t use it, because it’s still wrong.

public static IEnumerable<T> With<T>(this IEnumerable<T> source, T item)
{
    var collection = source as ICollection<T> ?? source.ToList();

    if (collection.IsReadOnly)
    {
        collection = collection.ToList();
    }
    
    collection.Add(item);

    return collection;
}

It works like a charm, but some things were bugging me about the code. I was talking with some of the guys after the Alabama Code Camp, and I lamented that I was violating the Liskov Substition Principle. We came to the agreement that it’s okay to violate principles if you do so purposefully with full understanding of why you are doing so. But something kept nagging me after I left there and continued the 8 hour drive back home to Columbia. It was more than one principle that I was breaking.

After I spent the night at a motel and was leaving, it suddenly dawned on me that I was truly violating a functional principle from my own C# Ninjitsu talk: Avoid Side Effects. What happens if you already have a list. It’s possible to have unintentional consequences in your code.

[TestMethod]
public void WithShouldNotHaveSideEffects()
{
    var strings = new List<string> { "This", "is", "a" };
    var strings2 = strings.With("test");
    Assert.IsTrue(strings2.Contains("test"));
    Assert.IsFalse(strings.Contains("test"));
}

This test fails because it’s not expected that the original sequence will be modified. It certainly isn’t modified when an array is passed in, why should it work any different with a list?

I knew I was in violation of two different principles: one agile, one functional. The code had to be refactored, so I started thinking about where I went wrong. I decided it wasn’t being enough like LINQ. There is a design guideline, and I’m not sure if it’s written down anywhere: if you return an IEnumerable<T>, Consider Yield. I thought about how I would solve the same problem using yield, and it turn out to be quite simple.

public static IEnumerable<T> With<T>(this IEnumerable<T> source, T item)
{
    foreach (T t in source)
    {
        yield return t;
    }

    yield return item;
}

I’m amazed that I found one common bug and violated two design principles in what started as a three line method. It is rather embarrassing, but I hope by sharing this that you will avoid this bug and apply the Liskov Substitution Principle and Avoid Side Effects in your own code.