Runtime: Proposal: (I)Dictionary<TK,TV>.GetValueOrDefault

[Eilon]

Scenario

In almost every app or library I write I almost always end up needing to write code like this, in at least one place:

``` c#
SomeClass returnValue;
return DictionaryOfSomeClass.TryGetValue(someKey, out returnValue) ? returnValue : null;

The reason that the `IDictionary<TKey, TValue>` contract throws from its indexer for a non-existent key are well-known: an alternative behavior of returning `null` is nonsensical if `TValue` is a value type. Returning `default(TValue)` would work, but would clearly be a misleading behavior.

But with dictionaries of class-types it is often desirable and expected to return `null` for a non-existent key.

## Proposal

```c#
namespace System.Collections.Generic
{
    public class Dictionary<TKey, TValue>
    {
        public TValue GetValueOrDefault(TKey key);
        public TValue GetValueOrDefault(TKey key, TValue defaultValue);
    }

    public static class CollectionExtensions
    {
        public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key);
        public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);

        public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key);
        public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
    }
}

Notes:

• There will be source-compile problem with ambiguity in all projects that already have such custom extension method GetValueOrDefault on IDictionary or IReadOnlyDictionary. Which is general problem for any extension method we add into CoreFX.

Original Proposal - replaced by the one above

I propose the following extension method (I don't like the name GetValueNoThrow, so that can be ignored for now). Please note that it applies only when TValue is a class, so it does not violate any behavior for value types.

``` c#
public static class ClassyDictionaryExtensions
{
public static TValue GetValueNoThrow_Ignore_The_Name(this IDictionary dictionary, TKey key)
where TValue : class
{
if (dictionary == null)
{
throw new ArgumentNullException(nameof(dictionary));
}
TValue value;
dictionary.TryGetValue(key, out value);
return value;
}
}

The new code would end up with one very-easy-to-read line of code:

``` c#
            return DictionaryOfSomeClass.GetValueNoThrow(someKey);

Discussion

Would others find such an extension method useful?

[JonHanna]

return DictionaryOfSomeClass.TryGetValue(someKey, out returnValue) ? returnValue : null;

You don't need the test here. You can just call TryGetValue and return returnValue either way, since in the case where you are explicitly returning null, returnValue is null.

[Eilon]

Ah good point, the original code could be:

c# DictionaryOfSomeClass.TryGetValue(someKey, out returnValue); return returnValue;

But I find that code to be kind of ugly too. It's taking two lines to express a rather simple concept.

[JonHanna]

It only saves a line if either combined with declaration into an initialisation, or in the tail position. When just assigning it's one line either way.

[ellismg]

Not making any judgments on the merit of the API to be include or not, but I was wondering if you considered removing the constraint that the TValue must be a class and making the method something like:

public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key)
public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue)

This feels more in line with LINQ for example.

[Eilon]

@JonHanna the return value is often passed into another method, or set on some other property, e.g. I want to do this:

``` c#
otherObj.SomeProp = _dictionary.GetValueNoThrow("someKey");

In that case it's actually two extra lines to write today:

``` c#
SomeType value;
_dictionary.TryGetValue("someKey", out value);
otherObj.SomeProp = value;

[Eilon]

@ellismg yeah that would be quite good as well. Definitely more LINQ-y.

[MgSam]

I always use a GetOrDefault extension method for Dictionary. It's far nicer to work with than TryGetValue.

[terrajobst]

The proposal looks good. I think what's missing is:

• Which type should we extend? Should we handle the read-only interfaces, should we handle the non-generic interface, too?
• Which type should it go into?

[Eilon]

@terrajobst I would think that just IDictionary<K,V> and IReadOnlyDictionary<K,V> would be sufficient. As to the non-generic IDictionary, well, it would be nice to have the symmetry, but I don't see that type used much anymore. Generics have been around since 2005 (.NET 2.0, ten years ago, wow, that's a long time ago!), so nearly every API since then uses the generic flavors.

As to the sponsor type, the type name I have no particular thoughts, but I'd love to have it in the System.Collections.Generic so it's available whenever Dictionary<K,V> or IDictionary<K,V> are used.

[JonHanna]

It's pointless on IDictionary as it's indexer is specified as returning null when the key isn't found, anyway. (Which was a great nuisance).

[loic-sharma]

Could we just make Dictionary's GetValueOrDefault be public instead of internal?

[ashmind]

I had this for a while in my extensions library (link), and there was one issue I ran into — having it on both IReadOnlyDictionary<> and IDictionary<> separately causes conflict when you try to invoke it on classes that implement both (e.g. concrete Dictionary<>).

I worked around by specifying explicit overload for Dictionary<>, but that obviously doesn't scale.

Since in .NET Core IDictionary<> still does not seem to implement IReadOnlyDictionary<> :confused: you might run into the same problem. I proposed potential language-level solution at dotnet/roslyn#5605, but it haven't got much traction so far.

[terrajobst]

With out variables coming in C# 7, does this API really provide that much value? I don't say it has zero value, or it is a bad idea, but the value certainly seems to be quite low..

We could probably just expose the existing method on Dictionary<K,V> but adding a new type to add an extension methods seems to be a bit heavy handed for a feature that's that narrow.

[Eilon]

@terrajobst the out variables feature also came in C# 6 and then went away 😄

What I don't recall is whether out variables allows you to use them in the same expression. Did it? I.e.:

``` c#
return DictionaryOfSomeClass.TryGetValue(someKey, out SomeClass returnValue) ? returnValue : null;

That syntax is still super clunky, but of course not as bad as what you have today.

I still think that this:

``` c#
return DictionaryOfSomeClass.GetValueOrDefault(someKey);

Is at least an order of magnitude nicer, easier to read, and is clear on the intention. Using the new C# 7 features (should it actually happen) is too full of "syntax" as opposed to just getting to the point.

[ianhays]

My opinion:

I try to be more of a minimalist when it comes to API: I'd rather provide the basic construction blocks as well as helper functions that either save paragraphs of code or simplify especially complex code. To me this fits neither of those criteria. I'd expect most people would be more likely to code this up themselves using TryGetValue than google it and learn about a new API that saves them a line or two.

[Eilon]

@ianhays I think that writing this:

```c#
return DictionaryOfSomeClass.GetValueOrDefault(someKey);

Instead of this:

```c#
return DictionaryOfSomeClass.TryGetValue(someKey, out SomeClass returnValue) ? returnValue : null;

Counts as "simplify especially complex code". With the code you have to write today, the intention of the code is not very clear. There's way too much syntax salt in there. And that's with a C# 7 feature that doesn't even exist yet.

I rather think the BCL should offer tons of utilities and helpers, so long as they have a clear purpose, lack ambiguity in terms of naming, etc.

[terrajobst]

It seems in many cases out variables will greatly remove the friction for APIs like TryGetValue. At the same time, it's not a replacement for the scenario @Eilon is after, which is providing a wrapper API for a dictionary. Consider the following, grossly simplified code:

```C#
class HttpHeaders
{
private Dictionary _httpHeaders;

public HttpHeaders(Dictionary<string, string> httpHeaders)
{
    _httpHeaders = httpHeaders;
}

public string Accept()
{
    return _httpHeaders.GetValueOrDefault("Accept");
}

public string Cookie()
{
    return _httpHeaders.GetValueOrDefault("Cookie");
}

public string ContentType()
{
    return _httpHeaders.GetValueOrDefault("Content-Type");
}

// ...

}

In other words, for cases where a method wants to return value from a dictionary, the current API pattern is quite ceremonial and out variables don't change that much:

 ```C#
public string ContentType()
{
    _httpHeaders.TryGetValue("Content-Type", out var result);
    return result;
}

Also, consider that out variables don't work in expression context, i.e. this doesn't work:

```C#
Dictionary dictionary = GetDict();
string[] keys = GetKeys();
var values = from key in keys
let exists = dictionary.TryGetValue(key, out var result)
select result;

Instead, you still have to clumsy code like this:

```C#
var values = from key in keys
             select dictionary.ContainsKey(key) ? dictionary[key] : null;

[karelz]

more likely to code this up themselves using TryGetValue than google it and learn about a new API

Ideally the API just shows in IntelliSense (typing Get for starter feels very natural to me). No need to search for the API online.
I wrote this utility function myself in quite a few projects. Having Dictionary.GetValueOrDefault in BCL would be helpful.

If we decide to add also IDictionary extension method (I don't have personal preference here), I think we should add general extension class in Collections for interfaces. I have a feeling this might not be the last useful extension method for Collections interfaces.

[jnm2]

I have the exact same IReadOnlyDictionary vs IDictionary extension method pain.

[karelz]

So here's the updated proposal then:

Proposal

```c#
namespace System.Collections.Generic
{
public class Dictionary
{
public TValue GetValueOrDefault(TKey key);
public TValue GetValueOrDefault(TKey key, TValue defaultValue);
}

public static class CollectionsInterfaceExtensions
{
    public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key);
    public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);

    public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key);
    public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
}

}
```

Won't there be a problem when 2 different extension methods match the call? Can someone please try it out?

[karelz]

Tried it, adding all extension methods will work as soon as we add the methods also on Dictionary.

It will fail to source-compile code with custom 'dictionary' which implements both IDictionary and IReadOnlyDictionary and does not have its own GetValueOrDefault method. Is that acceptable source-compile change? If not, I would suggest to keep IDictionary extension method
Note: UPDATED IReadOnlyDictionary -> IDictionary: based on usage investigation https://github.com/dotnet/corefx/issues/3482#issuecomment-263007579.

Updating proposal on the top and marking for API review ...

[svick]

@karelz I looked at some data regarding the usage of IDictionary and IReadOnlyDictionary:

1. IDictionary is much more commonly used on GitHub than IReadOnlyDictionary: 710 000 results for IDictionary vs. 14 000 results for IReadOnlyDictionary
2. Among the top 1000 projects on NuGet there are:
   
   
   
   • 147 public types that implement IDictionary, but not IReadOnlyDictionary
   
   
   • 3 public types that implement IReadOnlyDictionary, but not IDictionary: MailKit.UniqueIdMap, Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary and System.Collections.Immutable.IImmutableDictionary
   
   
   • 9 public types that implement both, all from MS: System.Collections.Concurrent.ConcurrentDictionary, System.Collections.ObjectModel.ReadOnlyDictionary, System.Collections.Immutable.ImmutableDictionary, System.Collections.Immutable.ImmutableSortedDictionary, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidationStateDictionary, Microsoft.AspNetCore.Routing.RouteValueDictionary, Microsoft.AspNetCore.Mvc.ViewFeatures.AttributeDictionary, System.Collections.Generic.SortedDictionary, System.Collections.Generic.SortedList
   
   

I think this shows that the IDictionary overload is much more useful and if only one overload is kept, then it should be the IDictionary one.

If MS can add an instance method version of this method to its types that implement both interfaces (or at least those where it's deemed that this method would be useful), then I think both overloads could be added.

[ashmind]

@svick While IReadOnlyDictionary is less used on NuGet, in my work projects I use it almost everywhere. IDictionary came first, so it is not surprising it is actively used, but I believe IReadOnlyDictionary is a better practice and people should be encouraged to use it if possible.

To me the best option is to have either both overloads, or not have this at all.

[karelz]

@svick ok, proposal updated - we will discuss it in the API review meeting.

[Eilon]

@karelz I don't see a "source compile breaking change" -- this is an entirely new API, right? At worst, code couldn't easily use the new API, but no existing code that compiles would stop compiling, would it?

[karelz]

@eilon of course you're right - I got somehow carried away thinking about making IDictionary implement IReadOnlyDictionary :( ... and it clouded my view on a simple thing like this one. Sorry for that. Updating top proposal ...

The only compat source-compile problem will be with already existing extension methods of the same name (GetValueOrDefault).
I hope I got it right this time. If not, please let me know ASAP.

[Eilon]

Yup that sounds right to me! Though, the statement about already-existing extension methods applies to any extension method anyone ever adds to anything, so I'm not too worried 😄

[karelz]

Yep, agreed (I had similar note in my reply earlier, but it somehow disappeared during my edits - so I added it at least to the top proposal as "Which is general problem for any extension method we add into CoreFX.").

[bartonjs]

https://github.com/search?utf8=%E2%9C%93&q=%22TValue+getvalueordefault%22&type=Code&ref=searchresults currently shows ~975 hits. (Removing the quotes jumps significantly, so it's probably a reasonable estimate of the number of implementations)

[danmosemsft]

Internal on Dictionary currently: https://github.com/dotnet/corefx/blob/73f38a79f792bd113a7d6359ca9180b1d2ba8da0/src/System.Collections/src/System/Collections/Generic/Dictionary.cs#L545

[karelz]

Approved as is (name of Extension class updated on the top to CollectionExtensions).

[AlexRadch]

@karelz I am working on this issue

[JonHanna]

Do out variables in C# 7 reduce the pressure for this? I does make the "bad" example at the beginning a single-line expression.

[Eilon]

@JonHanna it makes it a single-line gross expression 😄 I think it definitely mitigates, but in my view doesn't solve the problem entirely.

[justinvp]

We should reopen this issue until the CoreFX-side of the changes in dotnet/corefx#14521 are completed.

[karelz]

Agreed, thanks for reminder.

[karelz]

The PR dotnet/corefx#14521 seems to be abandoned. Unassigning the issue - is it again "up for grabs" for anyone to pick up.

[Thaina]

I think API should separate constraint for class and struct

I was propose like this

```C#
public static V? GetValueOrNull(this IDictionary dict,K key) where V : struct
{
V v;
return dict != null && dict.TryGetValue(key,out v) ? v : (V?)null;
}

public static V GetValueOrDefault(this IDictionary dict,K key,V defaultValue) where V : struct
{
return dict?.GetValueOrNull(key) ?? defaultValue;
}

public static V GetValueOrDefault(this IDictionary dict,K key) where V : class
{
V v;
return dict != null && dict.TryGetValue(key,out v) ? v : null;
}
```

[JonHanna]

@Thaina I can see how the V -> V? of the first could have an advantage, but why constrain the bottom two? The only thing I see the constraint adding is that it prevents some use and forces people to re-write their own if they need it.

[Thaina]

@JonHanna I just think in general we should not assume default struct value as default. We should always get null to check or ensure default value by ourself

should not be hard just always add ?? 0 or ?? default(AStruct) at call site

[JonHanna]

I just think in general we should not assume default struct value as default.

The calling code is the place where that can be decided as wisdom or folly.

I just think in general we should not assume default struct value as default.

It's absolutely impossible from a generic method that isn't constrained, because such a method can't call either of those methods at all.

[Thaina]

The calling code is the place where that can be decided as wisdom or folly.

I think we should guide this convention to be default coding behaviour

It's absolutely impossible from a generic method that isn't constrained

@JonHanna Could you please clarify?

What I mean is. I propose all those extension method with constraint by that reason. If we got a dictionary of struct then we should always use GetValueOrNull like this

```C#
var dict = new Dictionary();
var v = dict.GetValueOrNull(key) ?? 0;

var dictVS = new Dictionary();
var vs = dictVS.GetValueOrNull(key) ?? default(ValueStruct);

// or
var v0 = dict.GetValueOrDefault(key,0);
var vs0 = dictVS.GetValueOrDefault(key,default(ValueStruct));
```

[JonHanna]

What if we have a dictionary of T and T is not constrained?

[jnm2]

Why constrain either overload? I don't get it. Allow me to use three parameters for classes and two parameters for structs, if I so desire, as well as vice versa.

[Thaina]

@JonHanna Well, I get your point now, you right

@jnm2 Because we could achieve what you want with ?? operator and it better performance to use it for default class

If we allow default parameter on class dictionary. There would be someone code like this

```C#
var obj = dict.GetValueOrDefault(key,new ClassObject());

Which is not good. It will always initial new object without using. We should always use `??` like this
```C#
var obj = dict.GetValueOrDefault(key) ?? new ClassObject();

so if it not null it would skip the new flow

You might be experience developer so you could easily avoid this pitfall. But I think we should not have newbie could use it

[JonHanna]

If a newbie does that, it'll be sub-optimal but not break. Sub-optimal but working code is fine for a newbie. The unnecessary allocation isn't shooting yourself in the foot, it's wearing Italian leather dress shoes while cleaning your house; more expensive than necessary but not actually stopping you from getting the job done.

Never mind newbies. I wouldn't worry about that code a large percentage of the time. If we've a more efficient path that can be taken when it does matter, then I wouldn't worry about the churn of a few extra bytes when it doesn't.

[Thaina]

@JonHanna That's to say even we don't mind newbie if normally we should not use then I think we would rarely use that function. So it redundant in my opinion

[jnm2]

@Thaina there are cases where passing a value that you have sitting around is more efficient than branching your code with ??. In some cases it may be sub-optimal to use ??.

Plus you could make your same argument against structs, that new SomeStruct(x, y, z) could be a lot of work that should go after a ?? instead of using the default value parameter. Your own argument is an argument _against_ having the GetValueOrDefault for structs at all. To be logical with your argument, you'd have to make structs use GetValueOrNull so that everyone is forced to use ??.

What do you do if newbies do this?

var myInstance = new MyInstance(with, a, longish, constructor, so, it, is, on, its, own, line);
var x = dict.GetValueOrDefault(key) ?? myInstance;

Point being, I want freedom. @JonHanna is exactly right.

[Thaina]

@jnm2

there are cases where passing a value that you have sitting around is more efficient than branching your code with ??. In some cases it may be sub-optimal to use ??

Please enlighten me how you would return default value without branching internally

Plus you could make your same argument, that new SomeStruct(x, y, z) could be a lot of work that should go after a ?? instead of using the default value parameter

You right, I should not make that another GetValueOrDefault for struct. Should just have only nullable api

I agree that we should have freedom. It just that, should we contains it in common API? Because we still free to extend it
Well, if it really common then I would agree

[jnm2]

Please enlighten me how you would return default value without branching internally

There's an internal branch in _both_ cases, @Thaina. The default parameter results in one branch being used, internally. The ?? syntax results in two branches, the internal one, and the additional external one. :)

You right, I should not make that another GetValueOrDefault for struct. Should just have only nullable api

And casting to a nullable adds even more perf overhead in all cases. I think this would be an even worse decision. It was intended as satire. =)

Freedom should be given to let people consume any of these APIs at their own choosing with no struct or class constraints.

[Thaina]

@jnm2 I know that why I wrote it like that. Please accept your satire back and know that I never have a problem to be tit for tat

As I said. I just want constraint for common case. You still have freedom to extend your own if you really need. I just want to nominate common best practice to be in corefx

Is the default value for class is common enough? I just don't think so so I think we should kept minimal and avoid redundant here

[jnm2]

@Thaina I couldn't tell, but fair enough :'D

However, you keep bringing up extending. Can you demonstrate a proof of concept where I can use other non-dictionary extension methods from CollectionExtensions but supply my own GetValueOrDefault with no type constraints?

In my experience you can't extend this way. The compiler is unable to resolve which extension method to use and fails the build, unless you get rid of the using System.Collections.Generic; directive. But then you lose the ability to use any other extension methods in CollectionExtensions and you lose the ability to say List<> without qualifying the namespace or adding using List = System.System.Collections.Generic.List;, not to mention using Dictionary = System.System.Collections.Generic.Dictionary; using IEnumerable = System.System.Collections.Generic.IEnumerable; (IEnumerable could be problematic) and so on ad nauseum.

[Thaina]

@jnm2 Well, that's argument is understandable and I would agree with that. My case just work with generic dictionary

[jnm2]

@Thaina Cool. I guess it's a bit like the discussion on whether tuples should be mutable, where the team decided to be permissive so that more people with different viewpoints would be served.