Adopt nullable reference types

[bart-degreed]

The plan is to enable nullable reference types and review the nullability of all code (ie choose between nullable vs non-nullable references).
A prerequisite for this is .NET 5 because of its improved unconstrained generics support.

In the process, we'll likely want to make breaking changes to clarify the contract. For example IResourceContextProvider.GetResourceContext() today returns null when not found. Due to this loose contract, all callers are required to null-check or possibly face a NullReferenceException. So instead we'd like to offer ResourceContext GetResourceContext() (throws when not found) and ResourceContext? TryGetResourceContext (returns null when not found).

[ThomasBarnekow]

To stay in the pattern, I think TryGetResourceContext should have the following API:

bool TryGetResourceContext(string publicName, out ResourceContext? context);
bool TryGetResourceContext(Type resourceType, out ResourceContext? context);

[bart-degreed]

Hi @ThomasBarnekow,

I'm aware of that pattern, which was introduced before nullable value types and generics existed (so before C# 2.0). Over time, using ref/out parameters is considered an anti-pattern by many, because it obfuscates inputs and outputs. Today we are in a better place, where we can use typed tuples to return multiple values and use both nullable value- and reference types.

But Microsoft didn't want to break existing users, so they stuck with it and even made it less painful to use by introducing inline declarations (out var). We are breaking anyway, so backwards compatibility is not a concern here. I prefer to have a simple and clear signature, over the convoluted TryXXX pattern.

Finally, this is not trying to parse or validate anything, it is just a keyed lookup, which makes the pattern not a great fit here.

[ThomasBarnekow]

Hi @bart-degreed, I agree with your points. I was coming from the naming convention that points to a specific pattern with a specific method signature.

Would it be better to avoid the Try prefix and use a different naming convention if, for good reasons, you are using a different signature?

[bart-degreed]

I don't see why. The method is not called TryParse, so it is something else. Would a method named "TryEnrollStudentInCourse" be wrong too? I think not. What alternatives do you have in mind?

[ThomasBarnekow]

The TryXxx pattern is not only used for Parse and TryParse. It is generally used where DoSomething(TInput input): TResult might throw and TryDoSomething(TInput input, out TResult result): bool does not throw but returns false if unsuccessful.

[bart-degreed]

I'm not convinced that's the case. Can you share links that prove your assumptions, ie are there prominent coding guidelines that require developers to use that out bool signature when a method starts with the word "Try"?

Let's assume that my earlier example "TryEnrollStudentInCourse(Student, Course)" would post a message to a queue to enroll the student, but silently ignore the operation when the student has already enrolled in that same course, but log a warning when that student is already enrolled in a different course at the same date/time and send an email when the student is not eligible for the course. It would return void in all cases. My point is that "try" could mean anything and it may throw too, for example when null is passed in, when out of memory, when a stack overflow occurs, etc. Take this method for example, which starts with Try but throws when the current token is not a number.

[ThomasBarnekow]

Here are a few links to guidelines, posts, and questions:

• Microsoft exception design guidelines on Try-Parse Pattern
• Blog Post on The Try/Can-Pattern
• stackoverflow.com post on When is "Try" supposed to be used in C# method names

Here are a few sample methods, some of which have been added recently with .NET 5 or .NET Core 3.0:

• TryCreate
• TryFormat
• TryGetGuid
• TryGetRuneAt

I'm just saying that the prefix Try creates an expectation regarding a particular pattern that is being both widely and recently used.

public bool TryGetDecimal (out decimal value) requires further explanation. I found the following question on stackoverflow.com: Why doesn't System.Text.Json.JsonElement have TryGetString() or TryGetBoolean(). Some answers talk about why this and similar methods throw (two of three exceptions thrown by the corresponding Get methods).

[bart-degreed]

Good sources, you have convinced me. The pattern appears to be more prominently alive than I thought. So I'm okay with adopting the signature you proposed.

I'm a bit torn about whether it should throw on null. I've looked at some framework methods:

• MailAddress.TryCreate: throws on null: no
• UniqueId.TryGetGuid: throws on null: n/a, throws when out of range
• Rune.TryGetRuneAt: throws on null: yes, throws when out of range
• Half.TryFormatSpan: throws on null: n/a, throws on bad format specifier
• Int32.TryParse: throws on null: no
• Dictionary<>.TryGetValue: throws on null: yes

I tend to think it should, because looking up "nothing" or "I don't know" is an invalid question. But I don't think it should throw on an empty string.

What are your thoughts? Would this have any practical implications for JADNC users that I'm not aware of?

/cc @maurei

[bart-degreed]

Today I started refactoring JADNC signatures to the proposed Try/out pattern, but I find it makes code harder to read and consume. Doing only 4 files, I already stumbled on the following issues. For example:

public AttrAttribute TryGetAttributeByPropertyName(string propertyName)
{
    return propertyName != null && _fieldsByPropertyName.TryGetValue(propertyName, out ResourceFieldAttribute field) &&
        field is AttrAttribute attribute ? attribute : null;
}

Becomes:

public bool TryGetAttributeByPropertyName(string propertyName, out AttrAttribute attrAttribute)
{
    if (propertyName != null && _fieldsByPropertyName.TryGetValue(propertyName, out ResourceFieldAttribute field) &&
        field is AttrAttribute attribute)
    {
        attrAttribute = attribute;
        return true;
    }

    attrAttribute = null;
    return false;
}

And:

public ResourceType TryGetResourceType(string publicName)
{
    return publicName != null && _resourceTypesByPublicName.TryGetValue(publicName, out ResourceType resourceType)
        ? resourceType : null;
}

Becomes:

public bool TryGetResourceType(string publicName, out ResourceType resourceType)
{
    if (publicName == null)
    {
        resourceType = null;
        return false;
    }

    return _resourceTypesByPublicName.TryGetValue(publicName, out resourceType);
}

And consuming code:

Type effectiveIdType = idClrType ?? _typeLocator.TryGetIdType(resourceClrType);

Becomes:

Type effectiveIdType = idClrType;
if (effectiveIdType == null)
{
    _typeLocator.TryGetIdType(resourceClrType, out effectiveIdType);
}

And:

AttrAttribute attribute = resourceType.TryGetAttributeByPublicName(attributeName);

Becomes:

_ = resourceType.TryGetAttributeByPublicName(attributeName, out AttrAttribute attribute);

Note I'm using a discard in the last case, to express that I didn't forget to check the return value. Still, the inline declaration makes it harder to track what variables are in scope. And splitting it up in two lines does not sound appealing either.

When switching to nullable reference types, we'll need to use special attributes on the out parameters to steer the compiler in the right direction, whereas with the current signatures we only need to add ? to the return type.

Overall, I find that using this pattern, the need arises to introduce temporary variables and copying data to work around its limitations. It doesn't compose well, as the examples show. This gets worse when writing LINQ statements with let clauses, where the official advice is to introduce a private method to perform the conversion (which, of course, EF Core is unable to translate to SQL anymore).

So my current thinking is to keep things as they are and not introduce this pattern. Feedback welcome.

[ThomasBarnekow]

Again, my comment on Try was that it is (widely and recently) used with a specific pattern (the bool return type and an out parameter). Why not simply remove the prefix Try from the method names. For example:

AttrAttribute attribute = resourceType.TryGetAttributeByPublicName(attributeName);

becomes

AttrAttribute attribute = resourceType.GetAttributeByPublicName(attributeName);

Otherwise, I would expect to see this:

if (resourceType.TryGetAttributeByPublicName(attributeName, out AttrAttribute attribute))
{
    // Perform some action if successful
}

This would be comparable to:

AttrAttribute attribute = resourceType.GetAttributeByPublicName(attributeName);
if (attribute is not null)
{
    // Perform some action if successful
}

[wayne-o]

Why not `GetAttributeByPublicNameOrDefault` ?

…

On Mon, 11 Oct 2021 at 14:29, Thomas Barnekow ***@***.***> wrote: Again, my comment on Try was that it is (widely and recently) used with a specific pattern (the bool return type and an out parameter). Why not simply remove the prefix Try from the method names. For example: AttrAttribute attribute = resourceType.TryGetAttributeByPublicName(attributeName); becomes AttrAttribute attribute = resourceType.GetAttributeByPublicName(attributeName); Otherwise, I would expect to see this: if (resourceType.TryGetAttributeByPublicName(attributeName, out AttrAttribute attribute)) { // Perform some action if successful } This would be comparable to: AttrAttribute attribute = resourceType.GetAttributeByPublicName(attributeName);if (attribute is not null) { // Perform some action if successful } — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1029 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABLPDFFBA2ROEAMWVJC2D3UGLRDTANCNFSM5ATJZK7Q> .

[ThomasBarnekow]

Why not GetAttributeByPublicNameOrDefault?

Also fine or better! Do we need a prefix or postfix? If the answer is yes, the OrDefault postfix is definitely better than the Try prefix. If the return type is nullable, though, my question is why we need to append OrDefault at all (again, it is good if we need to indicate this). A return value of null should then be possible without mentioning it in the signature.

[bart-degreed]

We need to distinguish because we have both TryGetAttributeByPublicName and GetAttributeByPublicName in the same class. It could be GetAttributeByPublicNameOrNull then, which is more specific. What I dislike about that is it emphasizes the return value, which is just a detail. The point is to express that the operation may fail silently on unexpected input, instead of throw.

@ThomasBarnekow you've made your point on the continued usage of the pattern by Microsoft. I don't disagree with that, but I find it quite inconvenient. So I wonder whether we should conform to an unpleasant pattern, solely because Microsoft does. We're not Microsoft, for example, we don't prefix private static fields with s_ which Microsoft does very consistently in their code.

[bkoelman]

@dennisdoomen do you have an opinion on this? Are there standards or conventions I'm unaware of? What would you recommend?

[dennisdoomen]

I don't know enough about your library to provide any specific advice, but I can provide generic advice about what I would expect:

• GetAttributeByPublicNameOrDefault - Makes total sense to me and aligns with LINQ's API like FirstOrDefault.
• GetAttributeByPublicName - I would expect it to throw an exception if it can't find the attribute by that public name
• FindAttributeByPublicName - I would expect it to return null if it can't find the specified attribute
• TryGetAttributeByPublicName - This is a common convention. It would return false if it can't find the specific attribute. It would return true if it can and pass the attribute through the out parameter. However, if the publicName parameter is null or empty, I would expect an ArgumentNullException or ArgumentException.

[bart-degreed]

Thanks for sharing your insights @dennisdoomen, greatly appreciated. As usual, there is no single right solution, so I'm going to choose on a case-by-case basis.

For our resource graph lookups, I believe Find* is the most appropriate fit.