14
\$\begingroup\$

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?

\$\endgroup\$
7
  • 2
    \$\begingroup\$ "I thought about maybe using reflection to somehow iterate over all members of the theme instance ..." Did you make a working attempt already? Usually we don't write new code for you. It seems to be a reasonable idea doing so though. \$\endgroup\$ Jul 21, 2020 at 18:04
  • \$\begingroup\$ What type of project are working on ? ASP.NET ? \$\endgroup\$
    – iSR5
    Jul 21, 2020 at 22:26
  • \$\begingroup\$ It's an AvaloniaUI project \$\endgroup\$ Jul 21, 2020 at 22:27
  • \$\begingroup\$ @FrederikHoeft is Theme class a part of Avalonia ? or is it a user defined class ? \$\endgroup\$
    – iSR5
    Jul 21, 2020 at 23:01
  • \$\begingroup\$ It is a user defined class and it's expected to grow in the future when it comes to the number of public member variables \$\endgroup\$ Jul 21, 2020 at 23:08

6 Answers 6

15
\$\begingroup\$

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)

\$\endgroup\$
4
  • \$\begingroup\$ You can also use Linq for validation: public bool IsValid() => this.GetType().GetProperties().All(property => property.GetValue(this, null) != null) \$\endgroup\$ Jul 22, 2020 at 7:33
  • 1
    \$\begingroup\$ @PeterCsala I've included that in my answer, thank. \$\endgroup\$
    – iSR5
    Jul 22, 2020 at 7:49
  • 2
    \$\begingroup\$ Please cache those reflection properties, the way you wrote it it will call very expensive virtual reflection functions over and over every time you call this function (which we don't know how often it is). Use a static readonly PropertyInfo[] PropertiesToCheck = typeof(Theme).GetProperties(); \$\endgroup\$
    – Blindy
    Jul 22, 2020 at 15:09
  • \$\begingroup\$ @Blindy thanks mate, I have updated the code. \$\endgroup\$
    – iSR5
    Jul 22, 2020 at 17:07
16
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I would use the name IsValid because theme.IsThemeValid is needlessly wordy, and if (theme.IsValid) is closer to English. I'd also make it a property since methods are thought by many to be verbs, and I wouldn't want to call it GetIsValid(). \$\endgroup\$
    – phoog
    Jul 22, 2020 at 22:04
3
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$
#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();
        }

    }

}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please review the code given by the OP and describe why your code is better. This is a code review website and code only answers, might not be clear enough for the OP to understand why your code would be better \$\endgroup\$
    – Icepickle
    Aug 3, 2020 at 14:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.