Easy to maintain ways that helps code readability in C#?

Question

Say we start with a base class that states if an object is valid or not (included one interface and two child classes for discussion purpose), like:

interface IValid
{
    bool IsValid();
}

public abstract class BaseClass : IValid
{
    public bool Validity { get; protected set; } = true;

    public bool IsValid()
    {
        return this.Validity;
    }
}

public class ChildOfTrue : BaseClass
{
    public ChildOfTrue() => this.Validity = true;
}

public class ChildOfFalse : BaseClass
{
    public ChildOfFalse() => this.Validity = false;
}

Attempt #1 - null check is implementer's responsibility:

(My thoughts: null check, it's everywhere...)

public class TestCase
{
    public TestCase()
    {
        List<BaseClass> testSubjects = new List<BaseClass>
        {
            new ChildOfTrue(),
            new ChildOfFalse(),
            null
        };
        foreach (var testSubject in testSubjects)
        {
            if (testSubject != null && testSubject.IsValid())
            {
                /* Perform tests. */
            }
        }
    }
}

Attempt #2 - introduce extension:

(My thoughts: I will need to either remove IsValid() from the BaseClass - which creates another problem with the IValid implementation, or change the extension method name to something like IsNotNullAndValid() - which I can call IsValid() instead of checking the property... At least it's going to look a bit more elegant in the end.)

public static class BaseClassExtensionHelper
{
    public static bool IsValid(this BaseClass o)
    {
        if (ReferenceEquals(o, null)) return false;
        return o.Validity;
    }

    public static bool IsNotNullAndValid(this BaseClass o)
    {
        if (ReferenceEquals(o, null)) return false;
        return o.IsValid();
    }
}

Attempt #3 - introduce helper class instead of extension:

(My thoughts: while I can have the best of both choices - calling IsValid() and still name the method IsValid() - from extension, it's not going to look friendly at all - especially if someone wants to chain the method and the method is a conversion.)

if (BaseClassExtensionHelper.IsValid(testSubject)) { /* Perform tests. */ }

While beauty is in the eye of the beholder, any suggestion to how you would do it for code maintenance + readability and why? (Please give code example as it helps, a lot.)

Thanks!

Answer 1

TL;DR
You are only responsible to null-check your encapsulated logic (= hidden from an external caller). All other information is (by elimination) supplied by the external caller and therefore not your responsibility to null-check (it is the external caller's responsibility).

---

public class ChildOfTrue : BaseClass
{
    public ChildOfTrue() => this.Validity = true;
}

public class ChildOfFalse : BaseClass
{
    public ChildOfFalse() => this.Validity = false;
}

This is not good practice. Derived classes are created when they introduce structural differences from their base class. What you're doing is simply changing a value. This is no different from doing things like:

public class John : Person
{
    public John() => this.Name = "John";
}

public class Five : MyNumber
{
    public Five() : base(5) {}
}

However, it's possible that your example class is oversimplified for the purpose of example. Regardless, I do question your intention to override a boolean property's value based on some derived classes.

---

public bool IsValid()
{
    return this.Validity;
}

I don't see the point of this method. It's nothing more than a property accessor.

Why not add the property to the interface? The method has no added value anyway.

public IValid IValidity
{
    bool Validity { get; }
}

---

Attempt #2 - introduce extension:

If it's to avoid null issues, I can tell you that it's not going to help. Extension methods are matched on variable type (BaseClass). Even if your BaseClass variable is null, you will still be able to compile and run this.

Yes, you can technically wrap a null check in this method. But your solution is probably going to have more than one class. All classes are nullable, so the problem exists for all classes.

Your current approach would lead you to have to make a similar null-wrapping check for every method that exists in every class that exists. Or, at the very least, for every method or property that is the implementation of an interface.

But since this applies to every class, even if it doesn't have an interface, you should really be doing this on object level. Oh wait, but then you can't actually wrap your class-specific methods anymore!

Trying to create a wrapper method for every public method/property for every class/interface is an exercise in futility, will not lead to cleaner code, and is going to be maintenance hell when you get to a sizeable codebase.

---

Attempt #3 - introduce helper class instead of extension:

Extension methods are really just helper methods with a nicer syntax. There is no technical difference between #2 and #3.

---

Summarization

What you're trying to do here is go around the intention of the language. Consider this simple variable:

BaseClass myObj = new BaseClass();

It's important here to distnguish the inside (BaseClass methods and properties, what we call "encapsulated logic") from the outside (the client code that declared this variable). This distinction is important, because null exclusively lives on the outside.

An object cannot know if it's null. By definition, if this object exists, it's not null.

This is like trying to think "what would I be thinking if I didn't exist?". You wouldn't be. That's the entire point of defining "not existing".

The absence of something can only be detected by an outside party that observes the object (or lack thereof). An object cannot detect its own absence, since being able to check for an absence inherently requires an object to exist in the first place. The premise defeats the intention.

Attempt #1 is correct. However, there are syntactical improvement to be made that create cleaner code:

foreach (var testSubject in testSubjects)
{
    if (testSubject?.IsValid() ?? false)
    {
        /* Perform tests. */
    }
}

Sidenote: you can also do testSubject?.IsValid() == true instead. However, I find that == true SEEMS more redundant than it is. ?? false clearly communicates to the developer who reads the code that it's written to handle cases of null.

or, as an alternative approach:

foreach (var testSubject in testSubjects.Where(ts => ts != null))
{
    if (testSubject.IsValid())
    {
        /* Perform tests. */
    }
}

or, my preferred option: not checking for null (in certain situations).

In your example code, you willfully introduced null. In my opinion, the code should blow up in your face: you introduced nonsensical data.

Similarly, if this is a method which receives a list of objects from an external caller, that external caller gave invalid input and therefore needs to be made aware of that.

To summarize the intention of choosing to ignore null checks:

When the giver of input and the receiver of null reference exceptions is the same party, the exception can be considered good feedback. More casually, I call this "shit goes in, shit comes out, no big deal".

For example:

public string GetNameOfPerson(Person p) => return p.Name;

You supplied null to this method, which is obviously going to ensure that this method can't do what you ask it to do. You deserve the exception.

However, if the giver of input is different from the receiver of the NRE, that does mean you need to handle the null. For example:

public string GetNameOfPerson(int personId)
{
    var person = myDatabase.GetPerson(personId);

    return person.Name; //possible null reference
}

The external caller supplied an ID. He cannot know if the underlying database actually has a person or not. Therefore, he does not deserve to receive a null reference exception, as he is not the one who introduced the null.

In other words, you are only responsible to null-check your encapsulated logic (= hidden from an external caller). All other information is (by elimination) supplied by the external caller and therefore not your responsibility to null-check.

There are two solution here. Either you give a more sensical exception:

public string GetNameOfPerson(int personId)
{
    var person = myDatabase.GetPerson(personId);

    if(person == null) throw new Exception("Person could not be found.");

    return person.Name; //possible null reference
}

Or you return a default value:

public string GetNameOfPerson(int personId)
{
    var person = myDatabase.GetPerson(personId);

    return person?.Name ?? "John Doe";
}

The correct solution is contextual. Generally speaking, you'd want to avoid throwing exceptions, but there are valid use cases for throwing exceptions.

Answer 2

In my experience clearer is to have only one solution to a problem. In attempts 2 and 3 you have IsValid() on object and helper methods which basically do nothing more then call IsValid(). That could confuse developers working with your code.

For shorter null checking you can use null-conditional operator?.

if (testSubject?.IsValid() == true)
{
    /* Perform tests. */
}

And wait for nullable reference types in c# 8

Answer 3

In a vacuum I would say go with variant #1:
It is the caller's responsibility that he or she won't misuse such things, e.g. call functions or access members of Null Objects.

Now, there may be instances where you find yourself constantly testing if something is Null or not. This is the point where you should stop for some brief reflection: Why do I need to do all those Null checks? Is there some architectural issue, and if so, can I resolve it? Having to perform constant Null checks is a code smell.

If you find that you cannot avoid the Null checks for some reason (maybe your 3rd party library vomits lists that contain many Nulls) you may be able to encapsulate the issue - create wrapper class which sanitizes the Nulls in the Lists (e.g. replacing them, removing them, etc.). If you yourself are the source of Nulls you may consider using e.g. a null object pattern instead.

In general though, unless you have very good reasons, you should avoid passing Null around in your program.