Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Nullable.TryGetValue #18260

Closed
jamesqo opened this issue Aug 23, 2016 · 6 comments
Closed

Proposal: Nullable.TryGetValue #18260

jamesqo opened this issue Aug 23, 2016 · 6 comments
Milestone

Comments

@jamesqo
Copy link
Contributor

jamesqo commented Aug 23, 2016

A common pattern when working with nullables is to do something with the value if the nullable has one, or do something else if it doesn't. This is most commonly written like this:

int? nullable = Foo();

// ...

if (nullable.HasValue) // or != null
{
    DoSomething(nullable.Value);
}
else
{
    DoSomethingElse();
}

The problem here is that even though we know that the nullable must have a value within the HasValue block, the compiler can't formally assume that and we have to explicitly unwrap it using Value. It would be nice if we provided a TryGetValue method on Nullable to aggregate these 2 operations (checking if the value exists, then unwrapping it) into one:

int? nullable = Foo();

int value;
if (nullable.TryGetValue(out value))
{
    DoSomething(value);
}
else
{
    DoSomethingElse();
}

// Even better...

int value;
if (Foo().TryGetValue(out value))
{
    DoSomething(value);
}
else
{
    DoSomethingElse();
}

As you can see it's possible to write more concise code with TryGetValue since we only need the nullable once, and it doesn't need to get saved in a local variable.

This will work even better when C# 7 introduces out var, since we can skip the explicit int value; declaration and instead write out int value.

Proposed API

namespace System
{
    public struct Nullable<T> where T : struct
    {
        public bool TryGetValue(out T value);
    }
}
@jamesqo
Copy link
Contributor Author

jamesqo commented Aug 23, 2016

@omariom, @GSPP, @jasonwilliams200OK

@tannergooding
Copy link
Member

@jamesqo, Why not do the following:

int? nullable = Foo();

if (nullable.HasValue)
{
    DoSomething(nullable.GetValueOrDefault());
}
else
{
    DoSomethingElse();
}

Calling GetValueOrDefault avoids the secondary check of HasValue (it literally just returns the value field). Where-as calling Value will first check the HasValue property and then throw an exception if it is false.

Also you can implement an extension method that does what you want (although you have to write a separate extension for each Nullable<T>)

public static class NullableExtensions
{
    public static bool TryGetValue(this int? nullable, out int value)
    {
        value = nullable.GetValueOrDefault();
        return nullable.HasValue;
    }
}

Note: I do like the cleanliness/readability of your proposal.

@omariom
Copy link
Contributor

omariom commented Aug 23, 2016

@tannergooding
Your methods works with a small change:

    public static bool TryGetValue<T>(this T? nullable, out T value) where T : struct
    {
        value = nullable.GetValueOrDefault();
        return nullable.HasValue;
    }
}

@svick
Copy link
Contributor

svick commented Aug 23, 2016

Not sure how useful this would be, since in C# 7.0, you should be able to use pattern matching for this:

if (Foo() is int value)
{
    DoSomething(value);
}
else
{
    DoSomethingElse();
}

@tannergooding
Copy link
Member

@svick, I think that really depends on the code generated for your sample.

I believe the isinst instruction used in pattern matching will generate sub-optimal code as compared to the proposed TryGetValue()

@jamesqo
Copy link
Contributor Author

jamesqo commented Aug 23, 2016

@tannergooding isinst is not generated, get_HasValue is. See here. Additionally the compiler generates a call to GetValueOrDefault rather than get_Value on the 'taken' branch, so the inefficiency you mentioned earlier is gone (although that is likely to be elided by the JIT anyhow).

Given that @svick's code sample essentially makes this whole thing redundant in C# 7, I guess it's best to close this.

@jamesqo jamesqo closed this as completed Aug 23, 2016
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 1.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants