3
\$\begingroup\$

This code is within a ViewModelBase class. I have a custom attribute defined that is meant to trigger methods when a property is changed. The following runs as the ViewModel is being constructed. The idea is to catalog the methods that should be triggered upon a property changed event based on the property name.

It works but I was wondering if there was a way to more efficiently identify methods with the custom attribute, since there are a lot of methods to loop through and generally only one or two methods that have the sought-after attribute.

ExecuteOnPropertyChangedMap = new Dictionary<string, List<MethodInfo>>();

foreach (var method in this.GetType().GetMethods(
    BindingFlags.Instance | BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy))
{
    var execAttributes = method.GetCustomAttributes(typeof(ExecuteOnPropertyChangedAttribute), true);
    foreach (ExecuteOnPropertyChangedAttribute execAttribute in execAttributes)
    {
        if (execAttribute == null) continue;

        foreach (string propertyName in execAttribute.PropertyNames)
        {
            if (!ExecuteOnPropertyChangedMap.ContainsKey(propertyName))
                ExecuteOnPropertyChangedMap.Add(propertyName, new List<MethodInfo>());

            ExecuteOnPropertyChangedMap[propertyName].Add(method);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Is there a way to do something like this and create a static dictionary at compile time? \$\endgroup\$ – Eric H May 16 '18 at 15:49
  • \$\begingroup\$ Not at compile time using reflection, reflection is all about runtime. To do this at compile time you would just directly call the methods from the setters of your properties. \$\endgroup\$ – Kelson Ball May 16 '18 at 21:56
  • \$\begingroup\$ That is the approach I'm trying to move away from, because there are cases where the property setters are in base classes but I want to trigger a method in a derived class. Nothing changes about the property logic except the need to call the method. Yes, I could make the property virtual and override it to add the method call but was trying to do something a little cleaner. \$\endgroup\$ – Eric H May 17 '18 at 15:17
3
\$\begingroup\$

It's true that reflection is fairly slow, but for one-time operations the performance isn't usually a deal-breaker.

This method runs once-per-type, "some time before the view model is instantiated"

// optional type alias for clarity in this post
using MethodMap = System.Collections.Generic.Dictionary<System.String, System.Collections.Generic.List<System.Reflection.MethodInfo>>;

public class ViewModelBase : ... 
{
    protected readonly MethodMap ExecuteOnPropertyChangedMap;

    const BindingFlags Flags = BindingFlags.Instance | BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy;
    protected static MethodMap GetMethodMap<T>() where T : ViewModelBase
    {
        // slight variation on your code here
        var t = typeof(T);
        var map = new MethodMap();
        // This is up to personal preference, but I recommend using 'var' and a constant for Flags to make your code as human readable as possible
        foreach (var method in t.GetMethods(Flags))
        {
            foreach (var execAttribute in method.GetCustomAttributes(typeof(ExecuteOnPropertyChangedAttribute), true))
            {
                if (execAttribute == null) continue;            
                foreach (string propertyName in execAttribute.PropertyNames)
                {
                    if (!map.ContainsKey(propertyName))
                        map.Add(propertyName, new List<MethodInfo>());        
                    map[propertyName].Add(method);
                }
            }
        }
        return map;
    }    

    // ...

    protected ViewModelBase(MethodMap map)
    {
        ExecuteOnPropertyChangedMap = map;
    }
}

Then in each of your view model types, you could add

public class MyViewModel : ViewModelBase
{
    private static readonly MethodMap map = GetMethodMap<MyViewModel>();

    public MyViewModel : ViewModelBase(map)
    {
        // ...   
    }
}

You can put the logic for reflection and building your map in your base class - but your base class doesn't know about its sub types. So each of your sub types is responsible for passing info about itself up to the base class to provide it with an instance-level reference to the sub types map.

By setting a private static readonly in the sub type we ask C# to 'calculate this some time before we need it but we don't care exactly when', as opposed to putting it in your constructor. If you put the call to the reflection code in your constructor it happens every time you construct the object (like you pointed out). With this it only happens once, 'some time' before you instantiate the first MyViewModel. In practice, the field will usually be set right before the first MyViewModel constructor is called.

\$\endgroup\$
  • 1
    \$\begingroup\$ How would you subclass MyViewModel? Creating another constructor and passing new MethodMap through the hierarchy feels inconvenient. I think a good middle ground is to keep all the maps in the base class (or in separate standalone class) in some static Dictionaty<Type, MethodMap> cache. I doubt that this dictionary will grow too much, so the cache[GetType()] lookup should be fairly cheap. \$\endgroup\$ – Nikita B May 17 '18 at 13:20
  • \$\begingroup\$ Not a bad idea @NikitaB. I was also thinking all these could be forced to evaluate asynchronously in app startup (it's a WPF app) but haven't thought that through fully yet. I've got some other fires burning but will give these more consideration within the next few days. \$\endgroup\$ – Eric H May 17 '18 at 15:22
  • 1
    \$\begingroup\$ The benefit of passing the map up the ctor hierarchy is that the compiler checks for you that it's been done. There are definitely other ways of doing this, but each should confirm that the MethodMap gets created - either via the compiler or with automated testing \$\endgroup\$ – Kelson Ball May 17 '18 at 16:42
  • \$\begingroup\$ @KelsonBall well, I would not rely on compiler too much. I mean this benefit goes out of the window as soon as you start subclassing. This class MyOtherViewModel : MyViewModel {} will compile. However if you add new methods to MyOtherViewModel, it will not work. So test are necessary either way. And I would rather test a single class which is responsible for creating a map for every view model, than write a test for every single viewmodel to check, that correct map was passed to constructor. \$\endgroup\$ – Nikita B May 18 '18 at 8:08
  • \$\begingroup\$ @EricH well, you can definitely build all the maps in one go using reflection (Assembly.GetTypes().BlahBlah). However you should measure the performance of this call. I doubt it will be too slow, but it probably depends on assembly size. \$\endgroup\$ – Nikita B May 18 '18 at 8:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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