1

Given the following class: 

public static class ComboModel
{
    public static Dictionary<int, int[]> Items()
    {
        return new Dictionary<int, int[]>
        {
            {
                1, 
                new[] { 
                    2,
                    3,
                    4  
                }
            }
        };
    }
}

I want to ensure immutability of the data in the class, but this feels javascript-y and has drawn some vague (but strong) criticism from my peers. What is wrong with it?

Mateen Ulhaq
  • 24,552
  • 19
  • 101
  • 135
Chris McCall
  • 10,317
  • 8
  • 49
  • 80
  • 10
    Well it allocates every time the method gets called. Maybe just use an [`ImmutableDictionary`](https://msdn.microsoft.com/en-us/library/dn467181(v=vs.111).aspx) instead? – vcsjones Mar 31 '16 at 20:37
  • 1
    It uses a lot of lines in the editor buffer too ;) – Dai Mar 31 '16 at 20:38
  • Constructing dictionaries is expensive, so initialize it once. Also, return IReadOnlyDictionary<> instead of Dictionary. This should open up some opportunities to refactor. – Trevor Ash Mar 31 '16 at 20:46
  • Your question doesn't quite specify what is immutable. Is it the dictionary itself, you want to be immutable, or do you just want to return a "fresh" yet mutable dictionary every time `Items()` is called, that the caller is free to mutate on their own? – vcsjones Mar 31 '16 at 20:50

1 Answers1

4

Just use ReadOnlyDictionary, and allocate it only once:

public static class ComboModel {
    private static readonly ReadOnlyDictionary<int, int[]> _items = new ReadOnlyDictionary<int, int[]>(new Dictionary<int, int[]> {
        {
            1,
            new[] {
                2,
                3,
                4
            }
        }
    });

    public static IReadOnlyDictionary<int, int[]> Items
    {
        get { return _items; }
    }
}

Note that not only you allocate new instance on each call, as others already mentioned - you also provide wrong feeling to the caller that he can modify that dictionary. In ReadOnlyDictionary there are no methods that can modify it. There are other benefits when caller knows structure it received cannot be changed: for example he can safely process items there with multiple threads.

Update: of course readonly collections do not magically make objects stored in that collections readonly - just the collection itself. If you want to ensure immutability of int[] arrays in your example, just make them readonly too:

public static class ComboModel {
    private static readonly IReadOnlyDictionary<int, ReadOnlyCollection<int>> _items = new ReadOnlyDictionary<int, ReadOnlyCollection<int>>(new Dictionary<int, ReadOnlyCollection<int>> {
        {
            1,
            Array.AsReadOnly(new[] {
                2,
                3,
                4
            })
        }
    });

    public static IReadOnlyDictionary<int, ReadOnlyCollection<int>> Items
    {
        get { return _items; }
    }
}
Evk
  • 98,527
  • 8
  • 141
  • 191
  • You may be wrong about the misleadingly-named "ReadOnlyDictionary" https://stackoverflow.com/questions/32438988/readonlycollection-are-the-objects-immutable – Chris McCall Mar 31 '16 at 20:52
  • So you mean you want to make not only dictionary, but all value arrays readonly also? – Evk Mar 31 '16 at 20:54
  • I believe the point is to stop anyone changing the data in underlying class and so returning a dictionary that allows the values in it to be changed breaks that. – Tim Rutter Mar 31 '16 at 20:59
  • Updated answer with fix preventing that too. – Evk Mar 31 '16 at 21:00
  • But you could still set any value to null or assign a new object to it therefore changing the data in the class. ImmutableDictionary is what is needed as that creates a copy. – Tim Rutter Mar 31 '16 at 21:01
  • @afrogonabike I think you cannot set key to null or new object in ReadOnlyDictionary - there is just no setter. Same for ReadOnlyCollection - you cannot modify items in it. – Evk Mar 31 '16 at 21:04