Are auto-adding indexers on 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;
        }
    }
}

Sample of how this may 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

With no other information about what you're doing, that looks like surprising behavior to me. I hope that you make it very clear from the context (i.e. name it an AutoInitializingDictionary or something) what's to be expected.

I would personally prefer to make this a method rather than an indexer; something like D.FindOrCreate. (I have the feeling there's an idiomatic name for a method that does this which I've temporarily forgotten.)

Answer 2

I think it is perfectly fine as long as this behaviour is made perfectly 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 perfect for counters and patterns like IDictionary<K,List<V>>; I can do

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

instead of the laborious:

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

Answer 3

I would say this violates two principles. 1) principle of least surprise. And 2) that getters shouldn't change anything.

I wouldn't expect to add a the pair {"foo", null} if foo doesn't exist in the colleciton.

x = collection["Foo"]

Answer 4

There's two problems that you should consider - both of which suggest this is a bad idea.

First, inheriting from the .NET BCL collection types is not generally a good idea. The main reason for this is that most methods on those types (like Add and Remove) are not virtual - and if you provide your own implementations in a derived class, they will not get called if you pass your collection around as the base type. In your case, by hiding the Dictionary<TK,TV> indexer property, you are creating a situation where a call using a base-class reference will do something different than a call using a derived-class reference ... a violation of the Liskov Substitution 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!

Second, and more importantly, creating an indexer that inserts an item when you attempt to find one is entirely unexpected. Indexers have clearly defined get and set operations - implementing the get operation to modify the collection is very bad.

For the case you describe, you're much better off creating an extension method that can operate on any dictionary. Such an operation is both less surprising in what it does, and also doesn't require creating a derived collection type:

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 5

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

Never

Is this reasonable, or contrary to best practices?

Contrary to best practices

That said, if the class is named appropriately, it'd be acceptable. I'd personally use GetOrAdd instead.

Answer 6

The primary reason I would be concerned is that it wouldn't be thread-safe. Multiple readers all attempting to possibly write to the Dictionary at once would require careful lock management that you wouldn't likely think of (or get right) at first.