Are automatic adding indexers to dictionaries and collections a good design decision?

Question

When is it acceptable for an indexer to automatically add items to a collection / dictionary? Is this reasonable or contrary to best practices?

public class I { /* snip */ } public class D : Dictionary<string, I> { public I this[string name] { get { I item; if (!this.TryGetValue(name, out item)) { item = new I(); this.Add(name, item); } return item; } } } 

An example of how this can be used in a collection:

 public class I { public I(string name) {/* snip */} public string Name { get; private set; } /* snip */ } public class C : Collection<I> { private Dictionary<string, I> nameIndex = new Dictionary<string, I>(); public I this[string name] { get { I item; if (!nameIndex.TryGetValue(name, out item)) { item = new I(name); this.Add(item); // Will also add the item to nameIndex } return item; } } //// Snip: code that manages nameIndex // protected override void ClearItems() // protected override void InsertItem(int index, I item) // protected override void RemoveItem(int index) // protected override void SetItem(int index, I item) } 

Answer 1

There are two issues that you should consider - both of which suggest that this is a bad idea.

First, inheriting from .NET BCL collection types is usually not a good idea. The main reason for this is that most of the methods of these types (for example, Add and Remove ) are not virtual - and if you provide your own implementations in a derived class, they will not be called if you pass your collection as a base type. In your case, hiding the indexer Dictionary<TK,TV> property, you create a situation where a call using a base class reference will do something other than a call using a derived class reference ... violation Liskov replacement principle :

 var derived = new D(); var firstItem = derived["puppy"]; // adds the puppy entry var base = (Dictionary<string,I>)derived; var secondItem = base["kitten"]; // kitten WAS NOT added .. BAD! 

Secondly, and more importantly , creating an indexer that inserts an element when you try to find one is completely unexpected . Indexers clearly defined get and set operations - implementing a get operation to modify a collection is very bad.

In this case, you will greatly improve the creation of an extension method that can work with any dictionary. Such an operation is no less surprising in what it does, and also does not require the creation of a derived type of collection:

 public static class DictionaryExtensions { public static TValue FindOrAdd<TKey,TValue>( this IDictionary<TKey,TValue> dictionary, TKey key, TValue value ) where TValue : new() { TValue value; if (!this.TryGetValue(key, out value)) { value = new TValue(); this.Add(key, value); } return value; } } 

Answer 2

Without additional information about what you are doing, this looks like amazing behavior to me. I hope that you are clearly out of context (i.e. Name it AutoInitializingDictionary or something else), as you might expect.

I personally would prefer to do this by a method rather than an index; something like D.FindOrCreate . (I have a feeling that there is an idiomatic name for a method that does this, which I temporarily forgot about.)

Answer 3

I would say that this violates two principles. 1) the principle of least surprise. And 2) that getters should not change anything.

I would not expect to add a pair of {"foo", null} if foo does not exist in the collection.

 x = collection["Foo"] 

Answer 4

I think it's fine if this behavior is made very clear. I have 2 decorator classes:

 public class DefaultValueDictionary<K, V> : IDictionary<K, V> { public DefaultValueDictionary(IDictionary<K, V> baseDictionary, Func<K, V> defaultValueFunc) { ... } } 

and

 public class ParameterlessCtorDefaultValueDictionary<K, V> : DefaultValueDictionary<K, V> where V : new() { public ParameterlessCtorDefaultValueDictionary(IDictionary<K, V> baseDictionary) : base(baseDictionary, k => new V()) { ... } } 

The second class is ideal for counters and templates, such as IDictionary<K,List<V>> ; I can do

 var dict = new ParameterlessCtorDefaultValueDictionary<string, int>(); ... dict[key]++; 

instead of laborious:

 int count; if(!dict.TryGetValue(key, out count)) dict[count] = 1; else dict[count] = count + 1; 

Answer 5

The main reason I would be interested is not to be thread safe. Many readers who try to write to the dictionary right away will require careful management of locks, which you could hardly think of (or get right) at first.

Answer 6

When is it acceptable for an indexer to automatically add items to a collection / dictionary?

Never

Is this reasonable or contrary to best practices?

Unlike best practices

However, if the class is named appropriately, that would be acceptable. Instead, I used GetOrAdd .