2
\$\begingroup\$

While working with dictionaries I've been always running into the same two problems (who wasn't?):

  • The indexer throws the KeyNotFoundExcepion if you try to retrieve an item with a non-existing key that does not tell you which key is missing.
  • The Add method throws the ArgumentExcetpion if you try to add an item with the same key and it also does not contain the key name.

There is also a secondary problem: there is no constructor accepting any IEnumerables so every time you create a dictionary from something non-dictionary you often end up looping twice twice over a collection (expecially with types derived from a dictionary) - and you cannot easily work with tuples.

I tried to fix it with my own implementation that is built on top of the normal dictionary. It adds two try/catches to add the key to the message and it adds IEnumerable constructors.

public class PainlessDictionary<TKey, TValue> : IDictionary<TKey, TValue>
{
    private readonly IDictionary<TKey, TValue> _dictionary;

    public static readonly IEqualityComparer<TKey> DefaultComparer = EqualityComparer<TKey>.Default;

    public PainlessDictionary()
    : this(DefaultComparer)
    { }

    public PainlessDictionary(IEqualityComparer<TKey> comparer)
    {
        _dictionary = new Dictionary<TKey, TValue>(comparer);
    }

    public PainlessDictionary(IEnumerable<KeyValuePair<TKey, TValue>> source)
    : this(source, DefaultComparer)
    { }

    public PainlessDictionary(IEnumerable<KeyValuePair<TKey, TValue>> source, IEqualityComparer<TKey> comparer)
    : this(comparer)
    {
        foreach (var item in source)
        {
            Add(item);
        }
    }        

    public PainlessDictionary(IEnumerable<(TKey Key, TValue Value)> source)
        : this(source, DefaultComparer)
    { }

    public PainlessDictionary(IEnumerable<(TKey Key, TValue Value)> source, IEqualityComparer<TKey> comparer)
    : this(comparer)
    {
        foreach (var item in source)
        {
            Add(item.Key, item.Value);
        }
    }

    public TValue this[TKey key]
    {
        get
        {
            try
            {
                return _dictionary[key];
            }
            catch (KeyNotFoundException ex)
            {
                throw new KeyNotFoundException($"The '{key}' key was not present in the dictionary", ex);
            }
        }
        set => _dictionary[key] = value;
    }

    public ICollection<TKey> Keys => _dictionary.Keys;

    public ICollection<TValue> Values => _dictionary.Values;

    public int Count => _dictionary.Count;

    public bool IsReadOnly => _dictionary.IsReadOnly;

    public void Add(TKey key, TValue value)
    {
        try
        {
            _dictionary.Add(key, value);
        }
        catch (ArgumentException ex)
        {
            throw new ArgumentException($"The '{key}' key has already been added.", ex);
        }
    }

    public void Add(KeyValuePair<TKey, TValue> item) => Add(item.Key, item.Value);

    public void Clear() => _dictionary.Clear();

    public bool Contains(KeyValuePair<TKey, TValue> item) => _dictionary.Contains(item);

    public bool ContainsKey(TKey key) => _dictionary.ContainsKey(key);

    public void CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex) => _dictionary.CopyTo(array, arrayIndex);

    public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator() => _dictionary.GetEnumerator();

    public bool Remove(TKey key) => _dictionary.Remove(key);

    public bool Remove(KeyValuePair<TKey, TValue> item) => _dictionary.Remove(item);

    public bool TryGetValue(TKey key, out TValue value) => _dictionary.TryGetValue(key, out value);

    IEnumerator IEnumerable.GetEnumerator() => _dictionary.GetEnumerator();

    public IDictionary<TKey, TValue> ToDictionary() => new Dictionary<TKey, TValue>(_dictionary);

}

There is nothing fancy about it so no examples this time. The question is as usual: is there anything (terribly) wrong with this class or maybe there is something missing? Is there anything that could be done better to make debugging even easier?

\$\endgroup\$
  • 1
    \$\begingroup\$ Perhaps I misunderstood your description, but I was expecting the Add method not to throw an exception but rather to silently replace the value for a duplicate key. Then again you could create a new AddOrReplace method for that. \$\endgroup\$ – Rick Davin Oct 27 '17 at 18:13
  • 1
    \$\begingroup\$ @RickDavin adding a new method (or even better, an extension) is actually a good idea. I wouldn't however change the behaviour of Add as this is how you expect it to work. \$\endgroup\$ – t3chb0t Oct 27 '17 at 18:15
  • \$\begingroup\$ But in the question you state throwing an exception is a problem \$\endgroup\$ – paparazzo Oct 27 '17 at 19:59
  • \$\begingroup\$ @Paparazzi right, one part of the sentence got lost in editing. Fixed. \$\endgroup\$ – t3chb0t Oct 27 '17 at 20:03
  • 1
    \$\begingroup\$ @RickDavin Note that the indexer already has the AddOrReplace behaviour (it was years before I realised this, so I don't know how common the knowledge is!). As t3chb0t says, Add should throw as it has always done. \$\endgroup\$ – VisualMelon Oct 28 '17 at 13:14
1
\$\begingroup\$

Don't have much to say about the code as it's rather simple and not much can be changed on most places, but I will give my two cents.

Besides the fact that you can add some extra info in the exception message, I don't see much benefit from using this class :/


The constructors are better off as extension methods as you can use your constructor only in your class, but you can use the extension method on any IDictionary<,> type. You currently don't offer any abstraction that you can inherit to fix that. But if you do, you wont be able to force the try/catch block's extra message.


Printing {key} doesn't look really useful to me, unless you're hardcoding those values that is. I assume you're not, so probably nameof(key) would be better?


You might also want to extract the logic for adding all elements from IEnumerable<> into a separate method, you might even publicly expose it, not sure about that tho.

\$\endgroup\$
  • \$\begingroup\$ Let me try to explain the idea with different words. I pretty often use types derived from the Dictionary and there is no way to create them from an IEnumerable from a query without creating two dictionaries. You end up with one that comes from ToDictionary and then you pass it to the base ctor and it loops over it again. About the key. It's not a hardcoded string ;-) It's an interpolated string. The code actually will do key.ToString() because I want to know which key is missing or already added - finding this key isn't usually easy ;-] \$\endgroup\$ – t3chb0t Oct 28 '17 at 6:56
  • \$\begingroup\$ Although know when I think of it again, an extension could also work and do the try/catch internally... I think I'll write it like ToDictionary<TKey, TValue, TDic>() where T : IDicitonary<TKey, TValue> new() and it should do too - oh, a lot of generics. This would at least work in cases where I cannot use this type because the indexer is still not covered. You try to retrieve a key that does not exist and you can debug for hours ;-) \$\endgroup\$ – t3chb0t Oct 28 '17 at 6:59
  • \$\begingroup\$ @t3chb0t I don't understand what you mean by there is no way to create them from an IEnumerable from a query without creating two dictionaries. ToDictionary is O(n) operation, it uses a single loop over the IEnumerable collection, where the 2 functors are invoked to get the key and value respectively, yours isn't much different right? You also don't accept any predicates and this is a bit restricting. What I meant about the hardcoded values, is that I'm not sure how you're going to know where the problem is, unless you know where you pass the exact value. \$\endgroup\$ – Denis Oct 28 '17 at 12:46
  • \$\begingroup\$ Suppose you have a class like this class Log : Dictionary<string, object>, now in order to initialize it from a collection you need to either write a new ToDictionary extension or a custom constructor running a loop from an enumerable or you end up calling the base constructor taking an IDictionary which leaves you with two loops over the same collection. \$\endgroup\$ – t3chb0t Oct 28 '17 at 12:49
  • \$\begingroup\$ If I now try to put the key Category in this dictionary and the exception tells me that Category already exists there, then I have a much better idea what went wrong and which key I tried to add multiple times. Knowing that some key already exists isn't of any help at all ;-) I want to know the exact key. \$\endgroup\$ – t3chb0t Oct 28 '17 at 12:50
0
\$\begingroup\$

I broke it down to extensions that now provide the additional try/catches that add the key to the message. So it's possible to use them with any IDictionary.

public static TDictionary ToDictionary<TKey, TValue, TDictionary>(
    this IEnumerable<(TKey Key, TValue Value)> source,
    TDictionary target
) where TDictionary : IDictionary<TKey, TValue>
{
    foreach (var item in source)
    {
        target.PainlessAdd(item.Key, item.Value);
    }

    return target;
}

public static TDictionary ToDictionary<TKey, TValue, TDictionary>(
    this IEnumerable<KeyValuePair<TKey, TValue>> source,
    TDictionary target
) where TDictionary : IDictionary<TKey, TValue>
{
    return 
        source
            .Select(x => (x.Key, x.Value))
            .ToDictionary(target);
}

public static TDictionary PainlessAdd<TKey, TValue, TDictionary>(
    this TDictionary target,
    TKey key,
    TValue value
) where TDictionary : IDictionary<TKey, TValue>
{
    try
    {
        target.Add(key, value);
    }
    catch (ArgumentNullException ex)
    {
        throw new ArgumentNullException("Dictionary key cannot be 'null'.", ex);
    }
    catch (ArgumentException ex)
    {
        throw new ArgumentException($"The '{(key == null ? "null" : key.ToString())}' key has already been added.", ex);
    }

    return target;
}

public static TValue PainlessItem<TKey, TValue>(this IDictionary<TKey, TValue> source, TKey key)
{
    try
    {
        return source[key];
    }
    catch (KeyNotFoundException ex)
    {
        throw new KeyNotFoundException($"The '{key}' key was not present in the dictionary", ex);
    }
}

Having these extensions I now could reuse them with my own dictionary. The constructors became as simple as:

public PainlessDictionary(IEnumerable<(TKey Key, TValue Value)> source, IEqualityComparer<TKey> comparer)
: this(comparer)
{
    source.ToDictionary(this);
}

and the indexer and the Add method are now one-liners too:

public TValue this[TKey key]
{
    get => _dictionary.PainlessItem(key);
    set => _dictionary[key] = value;
}

public void Add(TKey key, TValue value) => _dictionary.PainlessAdd(key, value);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.