Search code examples
c#mvvmreflectionconventions

Reflection: Invoking derived property getters in base class


I ran into some code convention issues during integration of my code into a larger system. In the following example, BaseClass and DerivedClass1 are the previously existing large system, and DerivedClass2 is my code which was inherited from another base class before the merge.

public abstract class BaseClass
{
    protected BaseClass()
    {
        foreach (var property in this.GetType().GetProperties())
        {
            var value = property.GetValue(this);
            if (value is PropertyEnvelop)
            {
                (...)
            }
        }
    }
}

public class DerivedClass1 : BaseClass
{
    public PropertyEnvelop MyProperty { get; }
        = new PropertyEnvelop("MyPropertyName", typeof(int), default(int));    
    // this is fine, because property initializers run in reverse order:
    // 1. Derived class property initializers
    // 2. Base class property initializers
    // 3. Base class constructor
    // 4. Derived class constructor
    // see link
}

public class DerivedClass2 : BaseClass
{
    public ComplexItem Item
        { get { return ComputeComplexItem(SimpleItem1, SimpleItem2); } }
    // this is BROKEN, because stuff like SimpleItem1, SimpleItem2
    // and ComputeComplexItem TOTALLY depend on some logic from
    // DerivedClass2 constructor and produce exceptions if the ctor
    // wasn't invoked yet.

    public DerivedClass2()
    {
        // prepare everything to bring DerivedClass2 in a valid state
    }
}

Link

(To be more specific, BaseClass is implementation of INotifyPropertyChanged for MVVM and it wants to do some auto-hookup for its notification properties.)

The question is, what practice here is bad? 1) Invoking derived class members via reflection in the base class, or 2) property getters and methods relying on the assumption that their class' constructor has been invoked already? Should I add null-checks and similar logic into all my properties, or should I disable this base class behavior (if I'm anyway not going to use PropertyEnvelop anywhere in my code)? I have a strong feeling that it is wrong to try invoking getters of properties of an instance which is not fully instantiated yet (as the second part of the mentioned blog says), but are there any official guidelines which recommend one option over another? (We have complex ViewModel classes with lots of internal logic to run during ctors.)

Fortunately for me, the real BaseClass has a flag to disable this behavior, but it is implemented as virtual bool property, and inherited classes just override it; so, BaseClass also calls its virtual members in its constructor, which (AFAIK) is also a bad code practice.


Solution

  • My company finally decided that their original pattern was wrong, and they will change the base class implementation. The suggestions are:

    1) if you mark all properties you want to auto-hook in base constructor with some [WarningAlertThisWillBeInvokedBeforeYourCtorAttribute], then you'll at least have the getter and the warning in one place. In Reflection, you can first check the attributes and only after that invoke the getter.

    2) If you don't want to introduce attributes (as in this case, where the auto-hook was introduced to fasten development as much as possible), you can at least check the type of the property before invoking the getter. And then you can put the warning into the type summary, like

    /// <summary>
    /// WARNING! Property of this type should be initialized inline, not in ctor!
    /// </summary>
    public class PropertyEnvelop
    {
        (...)
    }
    

    3) generally, it is not a good idea to invoke the property getter in base class ctor at all, so we'll seek for another solution.