In this entire post I talk about the implementation of a single property of type List<T> … that’s why I love software development.

I want to use a blog post by Mark Seemann as starting point for my post (btw, if you aren’t a regular reader of Mark’s blog already, you should definitely become one). So, you have to read Mark’s post first. I wait here …


… great, you’re back! In his post Mark writes:

The problem is aliasing. While named differently, subscribers and sut.Data.Subscribers is the same object.

However, it’s only a problem because subscribers and sut.Data.Subscribers point to the same mutable object. Mark mentions that in Fixing the problem. In this post I want to work on the implementation of StatusPolicyData. Maybe there’s a way to prevent the unexpected behavior from happening in the first place.

An Iterative Approach

Let’s have a closer look at StatusPolicyData. I guess it looks something like this:

public class StatusPolicyData
{
    public int UnitId { get; set; }
    public List<Guid> Subscribers { get; set; }
}

That’s how a lot of C# code looks like. Basically, the language is encouraging you to write code that way. If you want anything else you have to put additional effort into it (i.e. do more typing).
Well then, let’s put some effort into it. First, I enable nullable reference types (see last post) to prevent NullReferenceExceptions. Now, StatusPolicyData hast to look like this to get rid of all compiler warnings:

public class StatusPolicyData
{
    public int UnitId { get; set; }
    public List<Guid> Subscribers { get; set; } = new List<Guid>();
}

What’s next? Maybe it’s time to get a little help and install the Microsoft.CodeAnalysis.FxCopAnalyzers NuGet package. Once you’ve installed the analyzer you get the warning CA2227: Collection properties should be read only. I just make the warning go away:

public class StatusPolicyData
{
    public int UnitId { get; set; }
    public List<Guid> Subscribers { get; } = new List<Guid>();
}

Removing the setter is a breaking change, though. Let’s have a look at the original code from False negative again:

var subscribers = new List<Guid> { /* some Guid */ };
var sut = new StatusPolicy
{
    Data = new StatusPolicyData
    {
        UnitId = 123,
        Subscribers = subscribers
    }
};

With the removed setter we have to initialize StatusPolicyData like this:

var subscribers = new List<Guid> { /* some Guid */ };
var data = new StatusPolicyData
{
    UnitId = 123
};
data.Subscribers.AddRange(subscribers);
var sut = new StatusPolicy
{
    Data = data
};

That doesn’t improve readability. But the good news is, the change is sufficient to make HandleObserveUnitStatusStartsSaga in Mark’s post fail as expected. subscribers and sut.Data.Subscribers don’t point to the same object anymore. End of the story? Of course not! Let’s see what we can do to make it more readable. Why not add a constructor parameter?

public class StatusPolicyData
{
    public StatusPolicyData(List<Guid> subscribers)
    {
        Subscribers = subscribers;
    }

    public int UnitId { get; set; }
    public List<Guid> Subscribers { get; }
}

Now, we can initialize StatusPolicyData like this:

var subscribers = new List<Guid> { /* some Guid */ };
var sut = new StatusPolicy
{
    Data = new StatusPolicyData(
        subscribers: subscribers)
    {
        UnitId = 123
    }
};

That looks way better. Unfortunately, HandleObserveUnitStatusStartsSaga doesn’t fail with the last change to StatusPolicyData - so we’re back to a false negative. That’s not surprising. subscribers and sut.Data.Subscribers point to the same object all again.
In Fixing the problem, Mark edits the unit test HandleObserveUnitStatusStartsSaga. He defines subscribers as an IEnumerable and creates a new list by calling ToList() before assigning it to the property Subscribers. Here’s Mark’s code:

IEnumerable<Guid> subscribers = new[] { /* some Guid */ };
var sut = new StatusPolicy
{
    Data = new StatusPolicyData
    {
        UnitId = 123,
        Subscribers = subscribers.ToList()
    }
};

Why not move the fix into the implementation of StatusPolicyData? By doing that, we make it impossible to run into the same issue again - the next time it might as well cause some unexpected behavior in production:

public class StatusPolicyData
{
    public StatusPolicyData(IEnumerable<Guid> subscribers)
    {
        Subscribers = subscribers.ToList();
    }

    public int UnitId { get; set; }
    public List<Guid> Subscribers { get; }
}

With the above implementation of StatusPolicyData, the initial version of HandleObserveUnitStatusStartsSaga from False negative fails as expected. There’s only one more thing I’d like to change. In order to make StatusPolicyData more pleasant to use, I make the constructor parameter optional:

public class StatusPolicyData
{
    public StatusPolicyData(IEnumerable<Guid>? subscribers = null)
    {
        Subscribers = subscribers?.ToList() ?? new List<Guid>();
    }

    public int UnitId { get; set; }
    public List<Guid> Subscribers { get; }
}

That’s it. That’s a possible implementation of StatusPolicyData to prevent the unexpected behavior from happening:

  • Make the property read-only.
  • Have an (optional) constructor parameter of type IEnumerable.
  • Copy the elements to a new list instance in the constructor.

Do You Want to Go the Extra Mile?

Ok, there’s another issue we can work on. Have a look at the following test code (I’m using MSTest):

var subscribers = new List<Guid> { /* some Guid */ };
var sut = new StatusPolicyData(
    subscribers: subscribers)
{
    UnitId = 123
};

PrintIds(sut.Subscribers);

CollectionAssert.AreEqual(subscribers, sut.Subscribers);

Will the test succeed? It’s impossible to tell. It depends on the implementation of PrintIds. What if PrintIds is implemented like this:

void PrintIds(List<Guid> ids)
{
    ids.Clear(); //Enter the Devil ;-)
}

Obviously, the test will fail. Once again, the problem is aliasing. sut.Subscribers and ids point to the same mutable object.

Don’t Expose Mutable Objects

To make the unintended behavior impossible, we have to encapsulate the mutable list and only expose an immutable list. But we still need to be able to modify the list. Therefore, we have to add modification methods to StatusPolicyData:

public class StatusPolicyData
{
    readonly List<Guid> subscribers;

    public StatusPolicyData(IEnumerable<Guid>? subscribers = null)
    {
        this.subscribers = subscribers?.ToList() ?? new List<Guid>();
        Subscribers = this.subscribers.AsReadOnly();
    }

    public int UnitId { get; set; }
    public ReadOnlyCollection<Guid> Subscribers { get; }

    public void AddSubscriber(Guid subscriber) =>
        subscribers.Add(subscriber);

    public void RemoveSubscriber(Guid subscriber) =>
        subscribers.Remove(subscriber);

    public void ClearSubscribers() =>
        subscribers.Clear();
}

Given the above implementation, we have to modify the test code. You can’t just pass sut.Subscribers to PrintIds, you have to pass sut.Subscribers.ToList():

var subscribers = new List<Guid> { /* some Guid */ };
var sut = new StatusPolicyData(
    subscribers: subscribers)
{
    UnitId = 123
};

PrintIds(sut.Subscribers.ToList());

CollectionAssert.AreEqual(subscribers, sut.Subscribers);

Now the test succeeds independently from the implementation of PrintIds - if PrintIds doesn’t throw an exception. That makes your code way easier to understand. You don’t have to look up the implementations of the methods you are calling to make sure they don’t modify the lists you are passing.

There’s another benefit to having modification methods: you can implement some form of change notification (e.g. raise an event).

Immutability

As we’ve seen, mutability can cause some unexpected behavior. Do you like your code to behave as expected? Then I suggest to go for immutability - only fallback to mutability if necessary. Here’s what an immutable version of StatusPolicyData looks like:

public class StatusPolicyData
{
    public StatusPolicyData(
        int unitId,
        IEnumerable<Guid>? subscribers = null)
    {
        UnitId = unitId;
        Subscribers = (subscribers?.ToList() ?? new List<Guid>()).AsReadOnly();
    }

    public int UnitId { get; }
    public ReadOnlyCollection<Guid> Subscribers { get; }

    public StatusPolicyData AddSubscriber(Guid subscriber) =>
        new StatusPolicyData(UnitId, Subscribers.Append(subscriber));

    public StatusPolicyData RemoveSubscriber(Guid subscriber) =>
        new StatusPolicyData(UnitId, Subscribers.Except(new[] { subscriber }));

    public StatusPolicyData ClearSubscribers() =>
        new StatusPolicyData(UnitId);
}

What Would It Look Like in F#?

I’m glad you asked. When we’re talking about immutability, it’s only logical to consider using a language where immutability is the default case. Here’s an F# version of StatusPolicyData:

type StatusPolicyData =
    { UnitId : int
      Subscribers : Guid list }

module StatusPolicyData =

    let addSubscriber subscriber (statusPolicyData : StatusPolicyData) =
        { statusPolicyData with
            Subscribers = subscriber :: statusPolicyData.Subscribers }

    let removeSubscriber subscriber (statusPolicyData : StatusPolicyData) =
        { statusPolicyData with
            Subscribers = List.except subscriber statusPolicyData.Subscribers }

    let clearSubscribers (statusPolicyData : StatusPolicyData) =
        { statusPolicyData with
            Subscribers = [] }

What do you think about the different implementations? Is immutability the way to go? Or is the simplest implementation also the best one? Tell me in the comments.