A Tale of Two Principle Violations

by KodefuGuru 24. January 2010 23:57

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.

Tags: , , , ,

Kodefu

Comments

1/25/2010 7:22:42 AM #

Andrew Theken

You already know this, but be sure to:

yield break;

or evil things happen.

Andrew Theken United States

1/25/2010 8:21:23 AM #

Chris

yield break is unnecessary in the With method, as there's no reason to return control until the end of the iteration. But it is very useful for when you need to stop iterating.

Chris United States

1/25/2010 8:44:27 AM #

Eric S

>>it’s okay to violate principles if you do so purposefully with full understanding of why you are doing so.

It makes me smile to read this.  Just to be clear, I agree and would even go further at times.  You sometimes seem like the type that is very 'by-the-book', but I am glad to be wrong   Smile

Eric S United States

1/26/2010 12:35:19 PM #

Eric S

>>If you haven’t figured it out, don’t feel bad. 40 people over 2 of my sessions haven’t answered it..

This is tricky; I know I was confused when we talked about this yesterday!

In a nutshell you are saying that when a string Array is passed as a IList<string>, calling Add() will fail because Arrays are a fixed size.

This is a disturbing error message.

Say then, if you then wanted to cast 'strings' to an IList to check the property IsFixedSize (rather than checking IsReadOnly on ICollection<T>) this would lead to another quirk...

Consider this code:


public static void AddFail(IList<string> strings)
{
    if (!strings.IsReadOnly) //IsReadOnly is True here...
    {
        strings.Add("Fail1");
    }

    IList stringIList = (IList)strings;

    if (!stringIList.IsFixedSize)  //IsFixedSize is True here...
    {
        strings.Add("Fail2");
    }

    if (!stringIList.IsReadOnly) //IsReadOnly is False here...
    {
        // Bug, because IsReadOnly on IList is false.
        strings.Add("Fail3");
    }
}


I know this is not a practical example, and casting like this is sloppy and dagerous. I also understand that IsReadOnly on IList and IsReadOnly on ICollection<T> are two different properties. In my opinion this is very confusing.

When you think about it, Array implementing: IList, ICollection and IEnumerable, coupled with IEnumerable<T> implementing IEnumerable and itself being implemented by both IList<T> and ICollection<T>, can lead to 'imbread family' situations (so to speak) where funny 'genetic' mistakes are now possible.  This of course can be solved by being mindful of what you are casting, but that argument can be made for many bugs.

Sorry for getting off the real topic here, but this point you made interested me.

Eric S United States

1/26/2010 5:46:49 PM #

Gavin

I was always annoyed that there wasn't an overload for Enumerable.Concat that took a single element, e.g.

  public static IEnumerable<T> Concat(this IEnumerable<T> first, T second) {
    // same as your With()
  }

Or that in Enumerable they defined a Singleton method (like in F#'s Seq) so you could write:

  IEnumerable<string> strings = new List<string> { "This", "is", "a" };
  strings = strings.Concat(Enumerable.Singleton("test"));

Gavin United States

1/26/2010 6:22:40 PM #

chris

You could use new[] { "test" }, but I prefer the single item interface for readability.

chris United States

2/8/2010 7:34:17 PM #

trackback

A Tale of Two Principle Violations

9efish.?????? - Trackback from 9eFish

9eFish

2/9/2010 3:58:20 PM #

s

hi..

s United States

Add comment




  Country flag

biuquote
  • Comment
  • Preview
Loading



Powered by BlogEngine.NET 1.6.0.0
Theme by Mads Kristensen

Whois KodefuGuru

Chris Eargle

Chris Eargle
.NET Community Champion

LinkedIn Twitter Technorati Facebook

MVP - Visual C#

 

INETA Community Champions
Friend of RedGate
Telerik .NET Ninja
Community blogs & blog posts

I am a #52er


World Map

RecentComments

Comment RSS

Tag cloud

Disclaimer

The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.

© Copyright 2010