22

I'm creating a Dictionary object, using IEnumerable's ToDictionary() extension method:

var dictionary = new Dictionary<string, MyType>
    (myCollection.ToDictionary<MyType, string>(k => k.Key));

When it executes, it throws the following ArgumentException:

An item with the same key has already been added.

How do I get it to tell me what the duplicate key is?

H.B.
  • 166,899
  • 29
  • 327
  • 400
Robert Harvey
  • 178,213
  • 47
  • 333
  • 501
  • 2
    This isn't an answer to your question, but ToDictionary actually creates a dictionary. Why are you passing that dictionary into the constructor of another dictionary? And why specify generic type parameters you don't have to? I'd write your example without all that baggage as: `var dictionary = myCollection.ToDictionary(x => x.Key);` – Joel Mueller Apr 06 '11 at 22:34
  • 1. Because that is one of the constructor overloads for Dictionary. But you do have a point. – Robert Harvey Apr 06 '11 at 22:35
  • 2. Presumably, specifying the types eliminates any possibility of boxing, but I'm not sure about that. – Robert Harvey Apr 06 '11 at 22:36
  • It may not be the ideal way, but you can catch the ArgumentException and rethrow it as a new exception with more info such as the key, the original exception being the inner exception to this new one. – WorldIsRound Apr 06 '11 at 22:40
  • @WorldIsRound: I tried that, but the original `InnerException` is null, so there's no way to get to the duplicate key. Not via capturing the exception, anyway. – Robert Harvey Apr 06 '11 at 22:48
  • 3
    @Robert - The compiler infers the generic type parameters - if it can't, it will let you know. In this case, `foo.ToDictionary(...)` and `foo.ToDictionary(...)` compile to the exact same IL code, one just requires less typing. – Joel Mueller Apr 06 '11 at 22:48

4 Answers4

17

Get the duplicate keys:

var duplicateKeys =
  myCollection
 .GroupBy(k => k.Key)
 .Where(g => g.Count() > 1)
 .Select(g => g.Key);
Jonathan Parker
  • 6,705
  • 3
  • 43
  • 54
Guffa
  • 687,336
  • 108
  • 737
  • 1,005
  • 3
    Seems odd that the `ArgumentException` thrown by `ToDictionary()` doesn't include the failed key. How costly is your query? The collection is pretty big. – Robert Harvey Apr 06 '11 at 22:29
  • @Robert Harvey, actually that's perfectly normal: `ArgumentException` is a very general exception, it's not specific to dictionaries... – Thomas Levesque Apr 06 '11 at 22:31
  • @Robert Harvey: The grouping is fairly efficient, but with a large collection it will of course still be a lot of work. It can be done more efficiently by using a `Dictionary` and only count the occurances of each key instead of grouping them. – Guffa Apr 06 '11 at 22:36
  • 3
    @ThomasLevesque: There's no reason the exception could not have been `throw new ArgumentException("An item with the same key has already been added. Key: " + key.ToString())` While ToString() will not be meaningful for every key, I bet it would eliminate many man hours of debugging across all .NET Framework developers with hardly a downside. – Eric J. Oct 26 '15 at 00:31
  • @EricJ., yes, it could be included in the message. However there *are* downsides, although they're not immediately obvious. I suggest you read [this discussion on GitHub](https://github.com/dotnet/corefx/issues/1187) for more details. – Thomas Levesque Oct 26 '15 at 00:56
  • @ThomasLevesque: Interesting discussion. Looks like the gist is there is a pull request to show the key, if possible, that will eventually make its way in. As soon as I read `We should probably get rid of Enumerable.ToDictionary(). It is obviously a poor design choice. There is no reasonable expectation that anything enumerable should be convertible to a dictionary.`, I knew the rest of the discussion would be... interesting. – Eric J. Oct 26 '15 at 01:09
  • @EricJ. the github link is dead. do you know newer or achived link? – Raj Kamal Aug 30 '22 at 09:23
10

If your specific situation makes it okay to only insert one of a set of objects with duplicate Key properties into your dictionary, you can avoid this error entirely by using the LINQ Distinct method prior to calling ToDictionary.

var dict = myCollection.Distinct().ToDictionary(x => x.Key);

Of course, the above will only work if the classes in your collection override Equals and GetHashCode in a way that only takes the Key property into account. If that's not the case, you'll need to make a custom IEqualityComparer<YourClass> that only compares the Key property.

var comparer = new MyClassKeyComparer();
var dict = myCollection.Distinct(comparer).ToDictionary(x => x.Key);

If you need to make sure that all instances in your collection end up in the dictionary, then using Distinct won't work for you.

Joel Mueller
  • 28,324
  • 9
  • 63
  • 88
5

The failed key is not included because the generic dictionary has no guarantee that there is a meaningful ToString method on the key type. You could create a wrapper class that throws a more informative exception. For example:

//Don't want to declare the key as type K because I assume _inner will be a Dictionary<string, V>
//public void Add(K key, V value)
//
public void Add(string key, V value)
{
    try
    {
        _inner.Add(key, value);
    }
    catch (ArgumentException e)
    {
        throw new ArgumentException("Exception adding key '" + key + "'", e);
    }
}
phoog
  • 42,068
  • 6
  • 79
  • 117
  • I should have made the signature `void Add(string key, V value)` since my suggestion assumes that you're only interested in string keys. Answer edited. – phoog Apr 06 '11 at 22:57
  • ('because the generic dictionary has no guarantee...' Why would it need a guarantee?) – Iain Aug 02 '12 at 04:22
  • 3
    @Iain The default implementation of `ToString()` returns the name of the object's type. If the exception message were to say "the given key '{0}' is not in the dictionary", there's a very good chance that the message would say "the given key 'MyNamespace.MyType' is not in the dictionary". This is not helpful, since *all* instances of that type have the same string representation. If there were some sort of guarantee that the type supplied a more meaningful value for ToString, then a message like that would make sense. – phoog Aug 03 '12 at 04:52
1

The ArgumentException being thrown by the call to Dictionary.Add doesn't contain the key value. You could very easily add the entries to a dictionary yourself, and do a distinct check beforehand:

    var dictionary = new Dictionary<string, MyType>();
    foreach (var item in myCollection)
    {
        string key = item.Key;
        if (dictionary.ContainsKey(key))
        {
            // Handle error
            Debug.Fail(string.Format("Found duplicate key: {0}", key));
        }
        else
        {
            dictionary.Add(key, item);
        }
    }

This extra check should be fairly inexpensive because elements are stored by hash.

Scott Wegner
  • 7,263
  • 2
  • 39
  • 55
  • Yeah, that's true. I guess I was just being lazy with the `ToDictionary()` method. – Robert Harvey Apr 06 '11 at 22:46
  • Dictionary.Add says it throws upon a duplicate key. So you might (?) get a performance benefit by doing the Add straightaway and catching if it throws. – Iain Aug 02 '12 at 04:11
  • 2
    Catching an exception on purpose is almost always a bad idea performance wise, throwing and catching an exception is so complex that you will most of the time improve performance by adding an additional check. – Vincent Robert Nov 27 '12 at 11:08
  • 4
    While I agree that one should be mindful of the cost of throwing an exception, I don't agree here. The default functionality already throws an exception, so catching it only to add more information seems to be the right approach. Further, adding a duplicate to a dictionary is literally the exceptional case of a dictionary. Finally, I certainly don't agree with trading an exception with a Debug.Fail which will be removed in a release build, effectively allowing duplicates to be ignored. – b_levitt Oct 22 '15 at 17:56