6
\$\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\$
  • \$\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 '16 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\$ – Konrad Morawski Apr 1 '16 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 '16 at 17:23
11
\$\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\$
  • \$\begingroup\$ TryGetValue is an even better solution - efficiently handling the possibility of a key not being there, is the purpose of TryGetValue. \$\endgroup\$ – ToolmakerSteve Jan 22 '18 at 1:03
  • \$\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\$ – ToolmakerSteve Jan 22 '18 at 1:05
7
\$\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\$
  • 1
    \$\begingroup\$ Needs a better method name/ Current name does not imply that it will check first. \$\endgroup\$ – user9993 Mar 31 '16 at 13:14
  • \$\begingroup\$ @user9993: Method name updated..!! \$\endgroup\$ – SiD Apr 1 '16 at 9:21
  • \$\begingroup\$ I suggest AddOrIgnore. Rhymes well with other similar methods like GetOrAdd \$\endgroup\$ – nawfal Jul 26 at 11:02
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\$
  • \$\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 '16 at 17:21
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\$

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.