10
\$\begingroup\$

I'm pretty sure there's no better way to do this but I thought a consensus on here might be nice. Let me know what you think.

public static TValue GetValueOrDefault<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue = default(TValue))
{
    TValue value;
    return dictionary.TryGetValue(key, out value) ? value : defaultValue;
}
\$\endgroup\$
15
\$\begingroup\$

I'm pretty sure there's no better way to do this but I thought a consensus on here might be nice. Let me know what you think.

At first glance it seems to be ok but one could assume that he/she is always getting either the value or the default value. If one would get the default value for e.g if the dictionary is empty, why shouldn't he/she get the default value if the dictionary is null or the key is null ?

If this isn't the way the method should work, then you need some documentation which cleary states the purpose and any expected exceptions.

Now, what happens if the method is called either like so

Dictionary<string, int> dict = null;
dict.GetValueOrDefault("someKey");

or (assuming the method lives in the "DictionaryExtensions" class)

Dictionary<string, int> dict = null;
DictionaryExtensions.GetValueOrDefault(dict, "someKey");  

sure you get a NullReferenceException but is this the type of exception one would expect ? If I call a method with an parameter being null I would expect to get either no exception or to get an ArgumentNullException.

What about if the key is null ? Sure you get an ArgumentNullException but it takes some time because it is thrown at the FindEntry() method of the Dictionary and this is also shown in the StackTrace.

IMO it would be better to check this cases inside the GetValueOrDefault() method and throw the exceptions there.

public static TValue GetValueOrDefault<TKey, TValue>(this Dictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue = default(TValue))
{
    if (dictionary == null) { throw new ArgumentNullException(nameof(dictionary)); } // using C# 6
    if (key == null) { throw new ArgumentNullException(nameof(key)); } //  using C# 6

    TValue value;
    return dictionary.TryGetValue(key, out value) ? value : defaultValue;
}  
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ very good! but i think the Default word here means we like to get the default value if the item does not exist not throwing an exception... may you see my ans please \$\endgroup\$ – S.Serpooshan Aug 8 '18 at 7:12
  • \$\begingroup\$ Good answer. I would handle null value for the dictionary parameter as well, throwing ArgumentNullException, since we explicitly use that parameter, dereferencing it before calling the TryGetValue() method. But, since we don't really use the key parameter, we shouldn't check/throw, because we don't know the internals of the TryGetValue() method, which actually uses it, and how it handles null values. Maybe its implementation will change over time and it later handle null values. From that point of view, I would avoid checking parameters we don't use but just forwaring to other methods. \$\endgroup\$ – Mladen B. Jun 25 '19 at 10:33
  • \$\begingroup\$ I'd also change the dictionary parameter to be IReadOnlyDictionary instead of Dictionary, to cover a wider range of use-cases. \$\endgroup\$ – Mladen B. Jun 25 '19 at 10:34
4
\$\begingroup\$

Another way to handle null is to return the default value also in this case. It could be not acceptable in some scenarios, but I think usually it is normal expectation when you call the method GetValueOrDefault. The Default word here means you like to get the default value if the item does not exist not throwing an exception which again requires to handle it in the code and decrease the usability of this helper extension.

For example, in a web app if you want to show some values in a text input or span(label), it isn't important whether dic is itself null or it doesn't contain the key. So, in this case the extension can be written as following:

public static class DictionaryExtensions
{
    /// <summary> Gets the value of specified key. Simply returns the default value if dic or key are null or specified key does not exists.</summary>
    public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dic, TKey key, TValue defaultValue = default(TValue))
    {
        return (dic != null && key != null && dic.TryGetValue(key, out TValue value)) ? value : defaultValue;
    }
}
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ You don't need the null checks at all. It doesn't matter if the key is null and for the dic you can use the null-propagation operator: dic?.TryGetValue . \$\endgroup\$ – t3chb0t Aug 8 '18 at 7:09
  • 1
    \$\begingroup\$ @t3chb0t thanks for comment. but I test and ensure that the dic null check is necessary! but the key null check can be probably removed. I first write the code using dic?.TryGetValue but the whole code will not be simpler as we get use of Unassigned variable error. May you test and provide full code please? \$\endgroup\$ – S.Serpooshan Aug 8 '18 at 7:17
  • \$\begingroup\$ Oh, you're right... that's weird. I'll take a look... \$\endgroup\$ – t3chb0t Aug 8 '18 at 7:22
2
\$\begingroup\$

You can use:

dict.ContainsKey(key) ? dict[Key] : defaultValue

so you don't need to declare TValue.

|improve this answer|||||
\$\endgroup\$
  • 5
    \$\begingroup\$ However, you end up doing two dictionary lookups instead of one if dict contains key, making this twice as slow as the TryGetValue approach. \$\endgroup\$ – Pieter Witvoet May 31 '19 at 6:44
  • 1
    \$\begingroup\$ Its O(1) constant lookup , doesn't matter how many times you do it. \$\endgroup\$ – Gorakh Borude May 31 '19 at 19:05
  • 3
    \$\begingroup\$ No - a lookup being O(1) means that it takes the same amount of time regardless of how many items the dictionary contains. But two lookups will still take twice as much time as a single lookup. Or in other words: a method that performs n O(1) operations is itself O(n). \$\endgroup\$ – Pieter Witvoet May 31 '19 at 20:58
1
\$\begingroup\$

There is a potential issue when providing an argument to a method that would only be used in a certain condition. defaultValue is only returned when the dictionary does not contain an instance of TValue for the given key.

  • What if the default value is the result of an exhaustive algorithm or a service call? It would be a pitty to fetch this object when GetValueOrDefault would not require it.
public static TValue GetValueOrDefault<TKey, TValue>(
   this Dictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue = default(TValue))
{
    TValue value;
    return dictionary.TryGetValue(key, out value) ? value : defaultValue;
}

For this reason, I suggest an alteration to the method signature and a convenience overload for the initial situation. (Edited after remark below)

public static TValue GetValueOrDefault<TKey, TValue>(
   this Dictionary<TKey, TValue> dictionary, TKey key, Func<TValue> defaultValueSelector)
{
    // arg checking left out for brevity
    TValue value;
    return dictionary.TryGetValue(key, out value) ? value 
       : defaultValueSelector != null ? defaultValueSelector() : default(TValue);
}

public static TValue GetValueOrDefault<TKey, TValue>(
   this Dictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue = default(TValue))
{
     return GetValueOrDefault(dictionary, key, () => defaultValue);
}
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ This results in dict.GetValueOrDefault(key) being an ambiguous call, and it slows down the (more commonly used?) TValue variant. Also, callers that use expensive default values can easily use TryGetValue directly, so I don't think these trade-offs are worth it. \$\endgroup\$ – Pieter Witvoet May 31 '19 at 8:12
  • \$\begingroup\$ @PieterWitvoet (1) That's why it's never a bad idea to test code in fiddle before posting (I'll adapt it) (2) This is getting on philosophical territory. I find it hard to determine the trade-off here. \$\endgroup\$ – dfhwze May 31 '19 at 8:17
  • \$\begingroup\$ Why not let the eager variant use TryGetValue directly, instead of calling the lazy variant? The only 'trade-off' then would be a duplicate TryGetValue call, but that's hardly a problem. \$\endgroup\$ – Pieter Witvoet May 31 '19 at 9:11
  • \$\begingroup\$ @PieterWitvoet That would be the best option, avoiding unnecessary object creation and allowing for best performance. \$\endgroup\$ – dfhwze May 31 '19 at 9:13

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.