TransWikia.com

C# consecutive and redundant null checks

Code Review Asked by Frederik Hoeft on November 11, 2021

I am working on a C# application and I want to support multiple themes, so I wrote a ThemeManager class that can check, load and apply a specific theme. A theme is considered valid, when all of it’s member variables are set (not null). If a theme is invalid I want to throw an IncompleteThemeException. However the following code feels a bit stupid…

public static void Load(Theme theme)
{
    AccentBrush = theme.AccentBrush ?? throw new IncompleteThemeException();
    AccentBrushLight = theme.AccentBrushLight ?? throw new IncompleteThemeException();
    WarningBrush = theme.WarningBrush ?? throw new IncompleteThemeException();
    WarningBrushLight = theme.WarningBrushLight ?? throw new IncompleteThemeException();
    ErrorBrush = theme.ErrorBrush ?? throw new IncompleteThemeException();
    ErrorBrushLight = theme.ErrorBrushLight ?? throw new IncompleteThemeException();
    MainBackgroundBrush = theme.MainBackgroundBrush ?? throw new IncompleteThemeException();
    InactiveDecoratorBrush = theme.InactiveDecoratorBrush ?? throw new IncompleteThemeException();
    SecondaryBackgroundBrush = theme.SecondaryBackgroundBrush ?? throw new IncompleteThemeException();
    MainTextBrush = theme.MainTextBrush ?? throw new IncompleteThemeException();
    SecondaryTextBrush = theme.SecondaryTextBrush ?? throw new IncompleteThemeException();
    DefaultPasswordIndicatorBrush = theme.DefaultPasswordIndicatorBrush ?? throw new IncompleteThemeException();

    AccentColor = theme.AccentColor ?? throw new IncompleteThemeException();
    MainBackgroundColor = theme.MainBackgroundColor ?? throw new IncompleteThemeException();
    AccentColorLight = theme.AccentColorLight ?? throw new IncompleteThemeException();
}

I thought about maybe using reflection to somehow iterate over all members of the theme instance to check if one of them is null, but I’m unsure what the best solution to this would be in terms of readability, performance and extensibility in the future.

How can this be improved?

6 Answers

#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();
        }

    }

}

Answered by Protiguous on November 11, 2021

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.

Answered by Mitch on November 11, 2021

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.

Answered by sardok on November 11, 2021

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.

Answered by RobH on November 11, 2021

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)

Answered by iSR5 on November 11, 2021

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.

Answered by Heslacher on November 11, 2021

Add your own answers!

Ask a Question

Get help from others!

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP