4
\$\begingroup\$

Preliminary rant:

System.Collections.Generic.KeyNotFoundException : The given key was not present in the dictionary.

Oh. My.

So I finally got fed up with how default exception information is beyond useless.

For a part of the project I will mostly no longer use the this[TKey key] property to access my Dictionaryies but will use a custom extension method that actually tries to tell me WTF went wrong.

Preliminary rationale:

The system I have has a large "integration testing" suite in addition to more easily debuggable Unit Tests, and if things go southwards, it is hard to grok what went wrong, if all you have is a rather unspecific stack trace and a message telling you:

System.Collections.Generic.KeyNotFoundException : The given key was not present in the dictionary.

I would note that this system heavily relies on (string) mappings, where some kind of configuration error can easily trigger such errors in unexpected places.

Especially, there are quite a lot of places in the code, where different dictionaries will be consulted in the same code part / line, making the stacktrace basically worthless to pinpoint the exact problem. Think:

var attributeValue = model[Config.PartX].attributes[Config.Attribute42];

I will also note that I started out with hand coding the TryGetValue stuff, but this got unreadable pretty fast, so I'm looking forward to trying a slightly less specific, but more hopefully usable, approach.

Requirements

  • "Key Not Found" Exception shall contain information pertaining to:

    • local name of dictionary
    • number of items in dictionary
    • value of the key
  • The information shall be recorded by the logging framework (NLog) without further fiddling.

  • I feel I have to explicitly note this: The potential leaking of key value information is beyond irrelevant for this system. Please do not comment wrt. this aspect.

Code

If applicable, all my snippets basically fall under https://opensource.org/licenses/unlicense in the unlikely event anybody would like to copy this.

namespace MY.Project
{
    public static class DictionaryHelper
    {
        /// <summary>
        /// Same as `TValue this[TKey key] { get; }`, but with a better exception message
        /// containing the dictionary "name" (needs to be provided), number of entries and key value
        /// </summary>
        /// <returns>Value if found, throws KeyNotFoundException otherwise</returns>
        public static TValue GetExistingValue<TKey, TValue>(this IDictionary<TKey, TValue> dict, string nameOfDict, TKey key)
        {
            if (!dict.TryGetValue(key, out var val))
            {
                throw CreateKeyNotFoundException(dict, nameOfDict, dict.Count, key);
            }
            return val;
        }

        /// <see cref="GetExistingValue"/> for `IDictionary` above.
        public static TValue GetExistingValue<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dict, string nameOfDict, TKey key)
        {
            if (!dict.TryGetValue(key, out var val))
            {
                throw CreateKeyNotFoundException(dict, nameOfDict, dict.Count, key);
            }
            return val;
        }

        /// <summary>
        /// Provide separate explicit overload for the `Dictionary` class because this class implements both
        /// the IDict and IReadOnlyDict interface, making the overload ambiguous otherwise
        /// </summary>
        public static TValue GetExistingValue<TKey, TValue>(this Dictionary<TKey, TValue> dict, string nameOfDict, TKey key)
        {
            return GetExistingValue((IDictionary<TKey, TValue>)dict, nameOfDict, key);
        }

        private static KeyNotFoundException CreateKeyNotFoundException<T, TCount, TKey>(T dict, string nameOfDict, TCount count, TKey key)
        {
            return new KeyNotFoundException(
                    $"{nameOfDict} ({dict.GetType()}) (with #{count} entries) does not contain key <{key}>!"
                );
        }
    }

Questions

Beyond general feedback:

  • Is there any sane name for this extension method?
  • Would you make this an extension method?
  • Is there any other good way to capture the name of the dictionary variable?
  • Order of parameters? How to prevent messing up order of string nameOfDict and keyif TKey == string?
\$\endgroup\$
4
  • 3
    \$\begingroup\$ "If applicable, all my snippets basically fall under opensource.org/licenses/unlicense ..." This statement is futile. As soon you posted here your content gets owned by Stack Overflow Inc. and the license is Creative Commons (check the help center which one exactly). \$\endgroup\$ Commented Aug 28, 2020 at 15:19
  • 1
    \$\begingroup\$ Can't you just fix the places where multiple dictionaries are accessed on the same line to make the stack trace useful? \$\endgroup\$
    – slepic
    Commented Aug 28, 2020 at 15:22
  • \$\begingroup\$ @πάνταῥεῖ - No. The Code is copyright by me and, yes, I license it (to S.O. and its users) under CC, but there's no problem if I additionally also provide it under another license so that others can copy it without adhering to the CC license (which would require attribution and ShareAlike). \$\endgroup\$
    – Martin Ba
    Commented Aug 31, 2020 at 8:51
  • \$\begingroup\$ @sleptic - One could do this, but in release builds, the line number information often isn't fully accurate, so if the accesses are just a few lines apart, it still won't help. \$\endgroup\$
    – Martin Ba
    Commented Aug 31, 2020 at 8:54

1 Answer 1

2
\$\begingroup\$

A different way to skin this cat is to wrap the dictionary. Something along the lines of this

public class DictionarWrapper<TKey, TValue> : IDictionary<TKey, TValue>
{
    private readonly IDictionary<TKey, TValue> innerDictionary;
    private readonly string name;
    
    public DictionarWrapper(IDictionary<TKey, TValue> innerDictionary, string name)
    {
        // could throw if null or set to empty
        this.name = name ?? string.Empty;
        this.innerDictionary = innerDictionary;
    }

    public DictionarWrapper(string name) : this(new Dictionary<TKey, TValue>(), name)
    {
    }

Most of the methods you would just be chaining down back into the innerDictionary but the indexer function you would put your code

public TValue this[TKey key] 
{
     get {
         // either trap error or write own TryGetValue and skip calling the normal inner indexer 
         try
         {
            return innerDictionary[key];
         }
         catch (KeyNotFoundException ex)
         {                     
             throw new KeyNotFoundException($"{name} ({innerDictionary.GetType()}) (with #{Count} entries) does not contain key <{key}>!", ex);
         }
     }
     set => innerDictionary[key] = value;
}

You wouldn't need to change your code that is accessing the dictionary and still get the exception info you want. Plus if you pass in the name in the constructor then you don't need to worry about continuing to pass it in each time.

You could also add to the constructor(s) to make it like the normal Dictionary constructors then you just need to change where the code create the dictionary to be your wrapperr

I also feel wrapping a class is pretty standard in programming, see Decorator Pattern, plus for future maintenance and someone coming after you they don't need to know about a new extension method.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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