C# consecutive and redundant null checks

#nullable enable

namespace Examples {

    using System;
    using System.Collections.Generic;
    using System.Diagnostics;
    using System.Linq;
    using System.Reflection;

    public class Program {

        public static void Main( String[] args ) {
            var theme = new Theme();

            var valid = theme.IsValid( out var invalidPropertyNames );

            if ( valid ) {
                Debug.WriteLine( $"All properties on {nameof( theme )} are valid." );
            }
            else {
                foreach ( var name in invalidPropertyNames ) {
                    Debug.WriteLine( $"Found invalid property {name}." );
                }
            }
        }

    }

    public class Theme {

        /// <summary>
        ///     Cache the reflection once.
        /// </summary>
        private static readonly PropertyInfo[] _properties = typeof( Theme ).GetProperties( BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public );

        private Int32? Test1 { get; } = 1;
        private Int32? Test2 { get; } = null;
        private Int32? Test3 { get; } = 3;

        /// <summary>
        ///     Run a check to see if all properties are valid (not null).
        /// </summary>
        /// <returns>Returns true if all properties do not have a  null value.</returns>
        public Boolean IsValid() => this.IsValid( out _ );

        /// <summary>
        ///     Output a list of any invalid (null) properties.
        /// </summary>
        /// <param name="InvalidPropertyNames"></param>
        /// <returns>Returns true if all properties do not have a  null value.</returns>
        public Boolean IsValid( out IList<String> InvalidPropertyNames ) {
            InvalidPropertyNames = _properties.Where( info => info.GetValue( this, null ) == null ).Select( info => info.Name ).ToList();

            return !InvalidPropertyNames.Any();
        }

    }

}

The code is not too far off of the of execution and I take your reflection proposal as a means to work around limitations in the language. This is a fairly straightforward use case for code generation.

Something like T4 or AOP via PostSharp or similar could easily generate the same code without the potential typos, readability, and maintenance issues.

If you want your Theme to be unaware of what is valid or invalid which makes sense as ThemeLoader may be responsible for defining it, you may try writing something like ef core's fluent api.

Something like following;

    var validator = new ThemeValidator<Theme>();
    validator.Property(theme => theme.AccentBrush)
        .IsRequired();
    validator.Propert(theme => theme.AccentBrushLight)
        .IsRequired();
    validator.validate(theme); // raises exception

This may look like what you do as you have to define for each variable. But i think, this way is open for extension as different conditions other than null check become necessary.

Two things to think about:

1. Could you code this so it's impossible to create an invalid Theme instance thus not having to worry about it in this method?
2. How does a consumer know what is invalid about their instance?

For 1, you could simply have a constructor that takes all of the arguments and throws if any are invalid. A pattern that can also work well is having defaults - that way, a theme can just override part of the default theme rather than having to specify everything.

For 2, add some context to your exceptions:

throw new IncompleteThemeException("Default Password Indicator Brush was not specified");

However, I would probably lean towards having a mutable builder class with defaults and have a create method there. Consuming it could look like:

var builder = new ThemeBuilder(Theme.Default /* or existing theme etc. */)
     .WithErrorBrush(myErrorBrush)
     .WithAccentBrush(myAccentBrush);

// Now build it.
var theme = builder.Build();

That seems like it would make it easier for consumers to use a Theme as all instances would always be valid and you get the ease of use via the builder class. A nice bonus is that you can add new properties to the theme without breaking consumers. They get a sensible default that they can choose to override if needed. You don't just start throwing IncompleteThemeException.

Since your Theme class is a user defined class, you could do something like :

public class Theme
{   // Cache the reflection once, then reuse it
    private static readonly PropertyInfo[] _properties = typeof(Theme).GetProperties();
    
    // properties 
    
    public Theme() { }
        
    
    public bool IsValid()
    {
        foreach(var property in _properties)
        {   
            var value = property.GetValue(this, null);

            if (value == null)
            {
                return false;
            }
        }
        
        return true;
    }
}

Or if you are into Linq (thanks Peter Csala):

public bool IsValid() => _properties.All(property => property.GetValue(this, null) != null);

Then this should go like :

public static void Load(Theme theme)
{
    if(!theme.IsValid()) { throw new IncompleteThemeException();    }
    
    AccentBrush = theme.AccentBrush; 
    //... etc..
}

UPDATE : @Blindy suggested to use static reflection and reuse it instead, to avoid creating multiple reflection instances. Which I mostly agree. So, I've updated the code so it can reuse the reflection instance (this including the Linq version as well). (Thanks to Blindy)

It sure looks somehow clumsy like it is. You should place such stuff deep down the road, meaning you should have a method inside Theme e.g. IsThemeValid() where you can place all these null checks.

Hence you would only have one method to maintain instead of each occurrence where you pass a Theme.