The Dangers of Mutable Data

I recently came across a bug in some C# code that would never have been a problem if the data structures being used were immutable. The data consisted of a .NET dictionary with an integer key and a list as a value. The details below have been changed to protect the guilty.

The offending method takes customer and product objects as parameters and retrieves a list of available pricing for the customer from a dictionary using the product ID as the key. Once the list is obtained, any pricing items the customer does not qualify for are removed and the list is returned.

private readonly Dictionary<int, List<IPricing>> _map =

    new Dictionary<int, List<IPricing>>();

private readonly PricingQualificationStrategy _strategy =

    new PricingQualificationStrategy();



private List<IPricing> GetAvailablePricing(

    Customer customer, Product product)

{

    int productId = product.ProductId;



    if (_map.ContainsKey(productId))

    {

        List<IPricing> potentialPricing = _map[productId];

        potentialPricing.RemoveAll(p => !_strategy.IsQualified(p, customer));

        return potentialPricing;

    }



    return new List<IPricing>();

}

The issue is with the line that removes all unqualified pricing. Items are removed from the list referenced by the dictionary, not a copy, so subsequent calls to the method do not have access to the complete list. If, by chance, a customer is not qualified for any pricing, then the list will be empty. From that point on, the method will always return an empty list for the product regardless of customer qualification.

potentialPricing.RemoveAll(p => !_strategy.IsQualified(p, customer));

It is a subtle bug. When originally coded, the programmer most likely assumed the dictionary was returning a copy of the list. What makes it even worse is the method returns a reference to the mutable list which can be further changed by the caller.

Instead of storing a mutable list in the dictionary, values of type IEnumerable<IPricing> can be used. It will achieve the desired result, returning a list of pricing items, without introducing the possibility of the list changing unexpectedly. In addition, the Enumerable.Where() extension method in the System.Linq namespace can be used to return a read-only enumeration with only the qualified items. The returned enumeration is a copy, and the original is unaffected.

The updated code is below.

private readonly Dictionary<int, IEnumerable<IPricing>> _map =

    new Dictionary<int, IEnumerable<IPricing>>();

private readonly PricingQualificationStrategy _strategy =

    new PricingQualificationStrategy();



private IEnumerable<IPricing> GetAvailablePricing(

    Customer customer, Product product)

{

    int productId = product.ProductId;



    if (_map.ContainsKey(productId))

    {

        var potentialPricing = _map[productId];

        return potentialPricing.Where(p => !_strategy.IsQualified(p, customer));

    }



    return new List<IPricing>();

}

This is a simple yet instructive example of the hidden dangers of mutable data.