12
\$\begingroup\$

What is the cleanest way to use dictionaries and protect against an unhandled ArgumentException if the key already exists in the dictionary?

I'd like to say that I can always guarantee uniqueness upstream, but I'm dealing with legacy code and mildly corrupt data from an old bug in the system.

I find myself using this pattern frequently but wonder if there is a better way.

public void AddToDictionaryContainsKey(Dictionary<int,string> myDictionary, int myKey, string myValue )
{
    if (!myDictionary.ContainsKey(myKey))
    {
        myDictionary.Add(myKey, myValue);
    }
}

Maybe a try/catch block - but this seems less readable. The advantage is that if I ever do decide to log the problem I have a place to do it.

public void AddToDictionaryTryCatch(Dictionary<int, string> myDictionary, int myKey, string myValue)
{
    try
    {
        myDictionary.Add(myKey, myValue);
    }
    catch (ArgumentException e)
    {
        // keep on truckin'
    }
}

The dictionaries are relatively small (typically < 1000 entries) so I prefer readability over performance.

\$\endgroup\$
3
  • \$\begingroup\$ Your first snippet is the way you should be adding but checking first. It's also more readable than your second snippet. Throwing exceptions is orders of magnitude slower than your first snippet because among other things an exception needs to capture the entire stack. You could make a AddIfNotExist method if you really want. \$\endgroup\$
    – user9993
    Mar 31, 2016 at 13:16
  • 2
    \$\begingroup\$ The advantage is that if I ever do decide to log the problem I have a place to do it - and what prevents you from logging the problem in your first solution, every time when ContainsKey is already true? : ) I can't see the advantage \$\endgroup\$ Apr 1, 2016 at 10:05
  • \$\begingroup\$ There was a bug in the distant past that allowed duplicates to be stored. By eliminating the duplicates here the system will be self healing and the problem eventually goes away. Logging an error would be for information only, there would be no action recommended. \$\endgroup\$
    – Shmoken
    Apr 4, 2016 at 17:23

6 Answers 6

15
\$\begingroup\$

Generating and handling exceptions is considered to be an expensive operation, so if(! x.ContainsKey()) is better. Yeah, the code example I see in MSDN uses try/catch but that's to illustrate the exception not advocate that as "best practice." Documentation I've read is pretty adamant about not throwing exceptions needlessly.

And you don't need try/catch to trap the duplicate key; just an else to the above if.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ For this particular case, since OP doesn't mind if duplicates are silently eliminated, myDictionary(myKey) = value; is an even simpler solution - one line! If a duplicate comes along, it overwrites the previous entry, so that is a slight difference in behavior ("last caller wins") from what OP wrote ("first caller wins"). \$\endgroup\$ Jan 22, 2018 at 1:05
  • \$\begingroup\$ @ToolmakerSteve - For situations like this when you don't care what the value is, why is TryGetValue better than ContainsKey? \$\endgroup\$ Dec 14, 2019 at 1:00
  • \$\begingroup\$ @KyleDelaney - my mistake, you are right. ContainsKey is better than TryGetValue here - I was thinking of a different situation, where you do want the value. I have deleted my comment recommending TryGetValue. \$\endgroup\$ Dec 14, 2019 at 11:32
  • 1
    \$\begingroup\$ .NET Core 3.1 gives the fairly concise myDictionary.TryAdd(myKey, myValue); that you can ignore the return success indicator bool. \$\endgroup\$ Apr 20, 2020 at 14:02
10
\$\begingroup\$

How about creating an extension method..

static class Extensions
{
    public static void AddSafe(this Dictionary<int, string> dictionary, int key, string value)
    {
        if (!dictionary.ContainsKey(key))
            dictionary.Add(key, value);
    }
}

and calling it like this:

var students = new Dictionary<int, string>();
students.AddSafe(1, "Apple");
students.AddSafe(1, "Orange");
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Needs a better method name/ Current name does not imply that it will check first. \$\endgroup\$
    – user9993
    Mar 31, 2016 at 13:14
  • \$\begingroup\$ @user9993: Method name updated..!! \$\endgroup\$
    – SiD
    Apr 1, 2016 at 9:21
  • \$\begingroup\$ I suggest AddOrIgnore. Rhymes well with other similar methods like GetOrAdd \$\endgroup\$
    – nawfal
    Jul 26, 2019 at 11:02
  • \$\begingroup\$ value part should be a Func<value> for perf reasons. \$\endgroup\$
    – nawfal
    Jul 18, 2020 at 23:13
1
\$\begingroup\$

Something is missing in your question: does a duplicate entry is an error or a special case? This is the only question you should ask yourself.

If duplicates are not expected, use an exception (and don't catch it at this level). Performance is not an issue when an error occurs.

Otherwise, don't use exceptions. This would deceive other developers (they will think that a duplicate IS an error). It would also bother them each time they'll ask Visual Studio to stop on each exception.

\$\endgroup\$
1
  • \$\begingroup\$ A duplicate key is not really an error or special case. There was a bug in the distant past that allowed duplicates into the database, but using the dictionary and removing duplicates will clean up the problem. It is essentially self healing. \$\endgroup\$
    – Shmoken
    Apr 4, 2016 at 17:21
1
\$\begingroup\$

I would go with ContainsKey, but take it one step further and make it a generic extension method

public static void AddIfKeyUnique<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key, TValue value)
{
    if (!dictionary.ContainsKey(key))
    {
            dictionary.Add(key, value);
    }
}
\$\endgroup\$
0
\$\begingroup\$

The ContainsKey way is cleaner I think so I prefer it in any way.

About performance, it's depend if your case, take in mind that check for contains key and then add key is do almost the same twice. So if all add operations result as success, the try\catch will be faster also if just some false occurred, but in the most case, the ContainsKey will be faster.

\$\endgroup\$
0
\$\begingroup\$

Where you already have had for years excellent answers on what the idiomatic best practice is for the given use case, as a developer you will often find yourself needing to tweak that code somewhat in different situations (e.g. different key or value types in a different dictionary, or maybe even a different dictionary class altogether) and rather than re-writing every instance where the pattern is used, you'll want to DRY (don't repeat yourself) your code out to have a minimal, well-tested area where this particular functionality and behavior resides. To that end, here's a very small extension method class that a) uses IDictionary rather than Dictionary to develop to interfaces rather than implementations, b) adds generics to the mix rather than the int and string concretions, c) returns a bool in the case you need to know if a the add made it in or not, d) utilizes overloads such that either the key and value may be used directly, or any mixture of Funcs that return those parameters can be used.

public static class IDictionaryExtensions
{
    public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue value)
    {
        if (dictionary?.ContainsKey(key) == false)
        {
            dictionary.Add(key, value);
            return true;
        }

        return false;
    }
    
    public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, Func<TKey> keyFunc, TValue value) =>
        dictionary.TryAdd(keyFunc(), value);
    
    public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, Func<TValue> valueFunc) =>
        dictionary.TryAdd(key, valueFunc());
    
    public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, Func<TKey> keyFunc, Func<TValue> valueFunc) =>
        dictionary.TryAdd(keyFunc(), valueFunc());
}
\$\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.