Dictionaries and invalid keys: return the key in the exception?

Dictionaries and invalid keys: return the key in the exception?

• Davve

  Simply wrapping the call to ToString() isn't safe though. The main problem is that you can't anticipate what someone will do in a ToString() so you can't adequetely handle the exception. There are really 3 choices Dictionary would have in this case.

  1. Call it without any guards and just let the exception propagate back up to the user. My previous post delt with this approach, so I won't continue with it.
  2. Swallow the exception and just throw the KeyAlreadyPresentException as you would with no additional information.
  3. Catch the exception and use it as the inner exception on the KeyAlreadyPresentException

  For #2 and #3, consider if the ToString() operation threw a non-trivial exception such as OutOfMemoryException or StackOverflowException. I'd like to say that this will never happen in a ToString() but I have personally screwed up once and ended up with a StackOverflowException from a ToString().

  If the exception thrown is non-trivial then both #2 and #3 are wrong. #2 will just swallow the exception and essentially ignore the fact that it ever happened. #3 will propagate the exception but in a way that's not immediately visible to the user. Calling code would likely just check the outer exception and not bother with the inner (there's really no great way to catch based on the inner exception).

  What's even worse is that if the exception is a StackOverflow or an OutOfMemory Exception, #2 and #3 might simply fail to function as intended due to the CLR being in a bad state.

  I understand the point you're making for the general case. But library designers have to consider all of the corner cases that can (and very often do) occur.

  
  
  

• mogulty

  Hi

  Usually exceptions are generic errors that do not contain information about the context. Why don't you add the information when the exception is thrown You can catch the exception and get the key that your are trying to assign.

  Cheers

  
  
  

• Maurice Pelchat

  I don't work on that team but I can provide some speculation.

  1. By the time you get to this point, Dictionary has encountered what it considers an error condition worthy of an exception. It's job is to report this to the user. Calling ToString() possibly prevents this operation because anyone can overload ToString() and throw another exception. So in your code instead of getting a KeyAlreadyPresent exception the stack trace is in SetValue but your ToString() is throwing It's not very descriptive and would certainly be confusing to a user. After all, why would you expect SetValue to call ToString() on your Key
  2. What happens when Key is null Same as above expept you'd get a NullReferenceException. It's even harder since this is a generic method that is not constrained to only contain class types so you can't even do a direct check for null.

  
  
  

• Bachi

  Why not simply .ToString() the key before storing it or putting it in the exception message Yes, I know I could do this myself, but wrapping all those dictionary accesses in try/catch statements gets tedious. I actually wrote a GetItemByPk<TKey, TValue(Dictionary<TKey, TValue, TKey key) routine to do what I describe, but I'm guessing that might hurt performance a little bit. I don't get the rationalization for not being specific with the messages in exceptions thrown -- unless there's a significant performance cost, I would think that more descriptive exception messages would lead to faster debugging. Seeing as throwing an exception in and of itself is expensive, throwing in a .ToString() and string concatenation seems trivial. Or am I missing something
  
  

• Potato K

  Another subtle reason is that exceptions should be serializable. There is no (good) way to gaurantee the key passed into Dictionary is acutally serializable therefore they can't include it in the exception.
  
  

• EitanD

  Simply wrap the ToString() in a try...catch statement.  Writing ToString() implementations that allow for throwing of exceptions isn't good design IMHO, but I know it can happen inadvertently.  Noting that the object threw an exception upon an attempt to call ToString() would be sufficient; if it's null report that the value was null, otherwise just note an exception was thrown.  The performance cost shouldn't be an issue, as TryGetValue or ContainsKey should have been used if it were.
  
  

  
  

• Patrick Hampton

  Might be that the motivation for throwing the exception is that the exeptions are meant to protect the implementation from unsafe or sensless operations.

  The API documentation specify that a key should adhere to certain specs, != null and the availability of GetHashCode come to mind.The API consumer (you, me) is therefore responsible for validating data prior to calling the function or face the music (= exception thrown). The reality is that the calling code has all the context it needs to determine which key caused the exceptions, so the addition of that information to the exception stack is an unnecessary cost.

  You are right that derived exception classes might and do have extra fields to store exception type specific data, but in this case, since the workaround for these exception are a cheap (cheaper than an exception) call to ContainsKey, the addition of data to the exception was probably not deemed necessary. Even the documentation refers to this as a trivial case of ArgumentException (snipped from Dictionary Generic Class in the documentation):

  // The Add method throws an exception if the new key is
  // already in the dictionary.
  try
  {
  openWith.Add("txt", "winword.exe");
  }
  catch (ArgumentException)
  {
  Console.WriteLine("An element with Key = \"txt\" already exists.");
  }
  

  Nuri

  
  
  
• Dictionaries and invalid keys: return the key in the exception?