Dereference of a possibly null reference ... Can my code be simplified?-CodePudding

My project is .Net-6 Blazor WebAssembly (hosted) in C#. Can my code be simplified to avoid nullable warnings? I want the person's customer-ID in a page variable from the ApplicationUser object's Identity Name (variable is '_Name'). Thanks.

List<Person> listPersons = (List<Person>)(await PService.GetPersons()).ToList();
Person oPerson = new Person();
if (listPersons != null){
    oPerson = (Person)listPersons.Where(p => p.Name!.Equals(_Name)).FirstOrDefault();
}
if (oPerson != null) {
    _UID_CUSTOMER = oPerson.UID_CUSTOMER;
}

CodePudding user response：

You can avoid nullable warning by removing this setting from csproj file

<Nullable>enable</Nullable>

And have these settings

<PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>    
    <ImplicitUsings>enable</ImplicitUsings>
</PropertyGroup>

CodePudding user response：

When you work with the Nullable Reference Type feature, you need to consider, for every (reference) variable, whether that variable allows a null.

This is no different than value types, such as int. You wouldn't do

int a = 5;
if (a != null) { // ... }

because a can never be null. You'd need to use the int? datatype to even allow a to be null.

Granted, there are way to break the Nullable Reference Type feature - such as ignoring warnings.

Let's take your code and fix some problems. I'll add line numbers.

1  List<Person> listPersons = (List<Person>)(await PService.GetPersons()).ToList();
2  Person oPerson = new Person();
3  if (listPersons != null){
4      oPerson = (Person)listPersons.Where(p => p.Name!.Equals(_Name)).FirstOrDefault();
5  }
6  if (oPerson != null) {
7      _UID_CUSTOMER = oPerson.UID_CUSTOMER;
8  }

Line 1

await PService.GetPersons() returns an IEnumerable<Person>. Because there's no ?, that means that the whole object cannot be null. In addition, each element (each Person object within the stream) cannot be null. If you really expect PService.GetPersons() to give you data or null, the return type would be Task<IEnumerable<Person>?>.

The cast of the IEnumerable<Person> into a List<Person> is dangerous. You get an IEnumerable<Person>, an interface. The underlying collection could be a List, or it could be an Array, or something else that implements IEnumerable. Casting it to a List can lead to runtime errors when the implementation of PService.GetPersons() changes.

There's not much point of running ToList() after the cast to a List. It's already a list. In fact, assuming you didn't get a cast exception, this method would through an exception if the List was null. This eliminates the point of doing a null check at all.

So, here's a better Line 1:

IEnumerable<Person> people = await PSService.GetPersons();

Use the right plural for "person".
Keep the type IEnumerable<Person>, no need to cast to List if you're only going to use the stream once.

Line 2

You set the default of oPerson to a new instance of a Person, and the datatype (Person) says that it can never hold a null value. However, on line 4, you use FirstOrDefault, which the "Default" will be null. So we need to change the datatype to account for that.

In addition, we're going to rewrite line 4 so that line 4 always runs, and the initialization of the variable on line 2 is unnecessary.

In fact, this whole line is unneeded, because it would just be the varaible name. So remove it.

Line 3 and Line 5

There's no point in checking the nullability of listPersons (now called people), because you've told the compiler that it can't be null. Remove those lines.

Line 4

Inside the Where you have Name!.Equals(). The ! is the "null forgiveness" operator. Problem with that is that if Name is null, then .Equals() will throw an exception. Replace .Equals with ==. (This is all assuming that the datatype of Name is a string?).

The cast at the end is also unnecessary. FirstOrDefault will return a Person (or null), so casting it to the same data type is wasteful.

Person? oPerson = people.FirstOrDefault(p => p.Name == _Name);

Side note, I don't agree with making the "default" value from FirstOrDefault a new instance of Person. My opinion is that FirstOrDefault's default should be null. This to me makes semantic sense for your code. You're looking through a list to find a matching person. If you can't find one, then you get null, not some new empty person.

Lines 6, 7, and 8

These are fine.

However, you could simplify the lines if the value of _UID_CUSTOMER was already null before executing these lines. In that case, all the lines could be replaced with:

_UID_CUSTOMER = oPerson?.UID_CUSTOMER;

This means:

If oPerson is null, just use null
If oPerson is not null, use the value of UID_CUSTOMER

Again, this only works if you didn't care about the value of _UID_CUSTOMER before this line executes. If you only want to overwrite _UID_CUSTOMER only when oPerson is not null, change it back to the if statement.

So, putting it all together you get

IEnumerable<Person> people = await PSService.GetPersons();
Person? oPerson = people.FirstOrDefault(p => p.Name == _Name);
_UID_CUSTOMER = oPerson?.UID_CUSTOMER;