8
\$\begingroup\$

I have a component that I've used myself for quite some time, the short story is that it automagically implements a property change notification mechanism (very useful for implementing INotifyPropertyChanged for example, but can be useful in other ways as well).

There are other solutions available which use a similar technique (post-build IL "Weaving") but I feel that I have a few novel features and via a NuGet package I have made it's use as simple as I think it could possibly be.

The truth is I use this component all the time, I've done so on a number of successful projects and I feel like it really helped me, so I would like to make it available. If .NET/WPF devs out there have a free minute (it literally only takes a minute to get it working) I'd like them to take a look and let me know what they think.

Here's the nuts and bolts (source) and here's the NuGet package (the NuGet package is definitely the way to go).

This is how it works:

The NuGet package now covers .NET framework versions 4.0-4.6+.

  1. Create a WPF project in visual studio, Target any framework 4.0 - 4.6.2 (if there's any favorable response I'll set up other frameworks in the NuGet package, what I've done should mostly work for all of them)
  2. In the package manager type the NuGet command: install-package Mathtone.MIST. Visual Studio will prompt you to reload the project.
  3. This should set everything up, adding a reference to the Mathtone.MIST assembly (which only contains the attribute classes you will use to decorate your code), copying the Mathtone.MIST.Builder and Mono.Cecil (this is the framework that does the actual weaving of IL operations, a fine piece of work) assemblies to the NuGet package "tools" folder. Additionally, it executes a nonthreatening script that modifies your project file and adds the post build task.

Here is an example:

using Mathtone.MIST;

namespace Lightly.Misted {

    //THe "Notifier" attribute indicates that notification should be implemented for this class.
    //Using explicit (default) notification
    [Notifier]
    public class ViewModel {

        //Raises the default notification ("WillNotify")
        [Notify]
        public string WillNotify { get; set; }

        [Notify]
        public string WillAlsoNotify { get; set; }

        //Notification will not be implemented
        public string WontNotify { get; set; }

        //Raises notification for the "DefinitelyNotAlias" property 
        [Notify("DefinitelyNotAlias")]
        public string AliasNotify { get; set; }

        //Raises multiple notification events
        [Notify("WillNotify","ComplexNotify")]
        public string ComplexNotify { get; set; }

        //Notification target, any method visible to the notifying property.
        //Can be implemented in a base class.
        [NotifyTarget]
        protected void PropertyChanged(string propertyName) {
            //up to you.
        }
    }

    //Implicit notification will implement notification for all settable properties, unless they are explicitly excluded.
    [Notifier(NotificationMode.Implicit)]
    public class ViewModel2 {

        public string WillNotify { get; set; }
        public string WillAlsoNotify { get; set; }

        [SuppressNotify]
        public string WontNotify { get; set; }
        [Notify("DefinitelyNotAlias")]
        public string AliasNotify { get; set; }
        [Notify("WillNotify", "ComplexNotify")]
        public string ComplexNotify { get; set; }

        [NotifyTarget]
        protected void PropertyChanged(string propertyName) {
            //up to you.
        }
    }
}

The notify target method can be implemented in a base class. One thing I would mention is that when using implicit notification, I would recommend suppressing notification for properties you have implemented. Most of the time this will work, but officially I have to plead "no soportado."

If someone could give it a try and let me know what they think I would greatly appreciate it.

This is the source code (in its current state) of the core components:

These attribute classes in the Mathtone.MIST library which is added to the target project as a reference:

using System;

namespace Mathtone.MIST {

/// <summary>
/// Used to mark a property as a notification provider.
/// </summary>
[AttributeUsage(AttributeTargets.Property)]
public class NotifyAttribute : Attribute {
    public string[] NotificationSource { get; protected set; }

    /// <summary>
    /// Initializes a new instance of the <see cref="NotifyAttribute"/> class.
    /// </summary>
    /// <param name="sourceNames">Properties that will be passed to the cotification target method.</param>
    public NotifyAttribute(params string[] sourceNames) {
        NotificationSource = sourceNames;
    }
}

/// <summary>
/// Used on public properties in implicit notification scenarios for which notificaiton should NOT be implemented.
/// </summary>
/// <seealso cref="System.Attribute" />
[AttributeUsage(AttributeTargets.Property)]
public class SuppressNotifyAttribute : Attribute {

}

/// <summary>
/// Used to mark a class for automatic notification implementation;
/// </summary>
[AttributeUsage(AttributeTargets.Class)]
public class NotifierAttribute : Attribute {

    public NotificationMode NotificationMode { get; protected set; }

    public NotifierAttribute(NotificationMode mode = NotificationMode.Explicit) {
        this.NotificationMode = mode;
    }
}

/// <summary>
/// Used to identify the notification target method.
/// </summary>
[AttributeUsage(AttributeTargets.Method)]
public class NotifyTarget : Attribute {

}

/// <summary>
/// Use implicit or explicit implementation of notification;
/// </summary>
public enum NotificationMode {

    /// <summary>
    /// Properties must be marked with the <see cref="NotifyAttribute"/>
    /// </summary>
    Explicit,

    /// <summary>
    /// Notification will be implemented for all public properties declared within the class;
    /// </summary>
    Implicit
}
}

And the following from the Mathtone.MIST.Builder project, this isn't referenced by the target project, but is loaded by visual studio as part of a build task.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Mono.Cecil;
using Mono.Cecil.Cil;
namespace Mathtone.MIST {
[Serializable]
public class CannotLocateNotifyTargetException : Exception {
    public CannotLocateNotifyTargetException(string typeName) :
        base($"{Resources.CannotLocateNotifyTarget}: {typeName}") {
    }
}
[Serializable]
public class InvalidNotifyTargetException : Exception {
    public InvalidNotifyTargetException(string methodName) :
        base(String.Format(Resources.InvalidNotifyTarget, methodName)) {
    }
}
[Serializable]
public class InvalidNotifierException : Exception {
    public InvalidNotifierException() :
        base(Resources.NotifyAttributeCannotBeSet) {
    }
}
[Serializable]
public class BuildTaskErrorException : Exception {
    public BuildTaskErrorException(string typeName, Exception innerException = null) :
        base(String.Format(Resources.BuildTaskError, typeName), innerException) {
    }
}
/// <summary>
/// Alters IL assemblies after build and implements a notification mechanism.
/// </summary>
public class NotificationWeaver {

    string NotifyTargetName = typeof(NotifyTarget).FullName;
    string NotifierTypeName = typeof(NotifierAttribute).FullName;
    string NotifyTypeName = typeof(NotifyAttribute).FullName;
    string SuppressNotifyTypeName = typeof(SuppressNotifyAttribute).FullName;
    string assemblyPath;
    DefaultAssemblyResolver resolver;
    MetadataResolver mdResolver;

    string ApplicationPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location);

    /// <summary>
    /// Initializes a new instance of the <see cref="NotificationWeaver"/> class.
    /// </summary>
    /// <param name="assemblyPath">Path to the assembly which is to be altered.</param>
    public NotificationWeaver(string assemblyPath) {

        this.assemblyPath = assemblyPath;
        this.resolver = new DefaultAssemblyResolver();
        this.resolver.AddSearchDirectory(ApplicationPath);
        this.resolver.AddSearchDirectory(Path.GetDirectoryName(assemblyPath));
        this.mdResolver = new MetadataResolver(resolver);
    }

    /// <summary>
    /// Weaves the notification mechanism into the assembly
    /// </summary>
    /// <param name="debug">if set to <c>true</c> [debug].</param>
    public void InsertNotifications(bool debug = false) {

        bool mustSave = false;
        var assemblyDef = null as AssemblyDefinition;
        var readParameters = new ReaderParameters { ReadSymbols = debug, AssemblyResolver = resolver };
        var writeParameters = new WriterParameters { WriteSymbols = debug };

        //Load the assembly.
        using (var stream = File.OpenRead(assemblyPath)) {
            assemblyDef = AssemblyDefinition.ReadAssembly(stream, readParameters);
        }

        //Search for types and weave notifiers into them if necessary.
        foreach (var moduleDef in assemblyDef.Modules) {
            foreach (var typeDef in moduleDef.Types) {
                try {
                    mustSave |= ProcessType(typeDef);
                }
                catch (Exception ex) {
                    throw new BuildTaskErrorException(typeDef.FullName, ex);
                }
            }
        }

        //If the assembly has been altered then rewrite it.
        if (mustSave) {
            using (var stream = File.OpenWrite(assemblyPath)) {
                assemblyDef.Write(stream, writeParameters);
                stream.Flush();
            }
        }
    }

    /// <summary>
    /// Weaves the notification mechanism into the supplied type.
    /// </summary>
    /// <param name="typeDef">The type definition.</param>
    /// <returns><c>true</c> if the type was altered, <c>false</c> otherwise.</returns>
    /// <exception cref="System.Exception"></exception>
    protected bool ProcessType(TypeDefinition typeDef) {

        var rtn = false;

        //Search for a NotifyAttribute
        var notifierAttr = typeDef.CustomAttributes.FirstOrDefault(a => a.AttributeType.FullName == NotifierTypeName);

        if (notifierAttr != null) {

            //Use explicit mode if not otherwise specified
            var mode = NotificationMode.Explicit;

            //Locate the notification target method.
            var notifyTarget = GetNotifyTarget(typeDef);

            if (notifyTarget == null) {
                throw new CannotLocateNotifyTargetException(typeDef.FullName);
            }

            //Determine whether to use explicit/implicit notifier identification.
            if (notifierAttr.HasConstructorArguments) {
                mode = (NotificationMode)notifierAttr.ConstructorArguments[0].Value;
            }

            //Identify the name of the property/properties that will be passed to the notification method.
            foreach (var propDef in typeDef.Properties) {
                var propNames = GetNotifyPropertyNames(propDef);

                if (!ContainsAttribute(propDef, SuppressNotifyTypeName)) {
                    //In implcit mode implement notification for all public properties
                    if (!propNames.Any() && mode == NotificationMode.Implicit && propDef.GetMethod.IsPublic) {
                        propNames = new[] { propDef.Name };
                    }
                    if (propNames != null) {
                        InsertNotificationsIntoProperty(propDef, notifyTarget, propNames);
                        rtn = true;
                    }
                }
            }
        }

        //Recursively process any nested type definitions.
        foreach (var type in typeDef.NestedTypes) {
            ProcessType(type);
        }

        return rtn;
    }

    /// <summary>
    /// Gets the notification target method, market with a <see cref="NotifyTarget"/> attribute.
    /// </summary>
    /// <param name="typeDef">The type definition.</param>
    /// <returns>MethodReference.</returns>
    protected MethodReference GetNotifyTarget(TypeDefinition typeDef) {

        //Check each method for a NotifyTargetAttribute
        foreach (var methDef in typeDef.Methods) {
            if (ContainsAttribute(methDef, NotifyTargetName)) {
                var isValid = false;
                switch (methDef.Parameters.Count) {
                    case 0:
                        isValid = true;
                        break;
                    case 1:
                        isValid = methDef.Parameters[0].ParameterType.FullName == typeof(string).FullName;
                        break;
                    //case 2:
                    //  isValid = methDef.Parameters[0].ParameterType.FullName == typeof(string).FullName &&
                    //      methDef.Parameters[1].ParameterType.FullName == typeof(object).FullName;
                    //  break;
                    //case 3:
                    //  isValid = methDef.Parameters[0].ParameterType.FullName == typeof(string).FullName &&
                    //      methDef.Parameters[1].ParameterType.FullName == typeof(object).FullName &&
                    //      methDef.Parameters[2].ParameterType.FullName == typeof(object).FullName;
                    //  break;
                }
                if (isValid) {
                    return methDef;
                }
                else {
                    throw new InvalidNotifyTargetException(methDef.FullName);
                }
            }
        }

        //Notify target not found, search base type
        var baseType = typeDef.BaseType;

        if (baseType != null) {

            //Get the definition of the base type
            var baseTypeDef = mdResolver.Resolve(baseType);

            //Search recursively for a target
            var rtn = GetNotifyTarget(baseTypeDef);

            if (rtn != null) {

                //A target has been found, import a reference to the target method;
                rtn = typeDef.Module.ImportReference(rtn);
            }

            return rtn;
        }
        else {
            return null;
        }
    }

    /// <summary>
    /// Determines whether the specified definition is decorated with an attribute of the named type.
    /// </summary>
    /// <param name="definition">The definition.</param>
    /// <param name="attributeTypeName">Name of the attribute type.</param>
    /// <returns><c>true</c> if the specified definition contains attribute; otherwise, <c>false</c>.</returns>
    public static bool ContainsAttribute(MethodDefinition definition, string attributeTypeName) =>
        definition.CustomAttributes.Any(a => a.AttributeType.FullName == attributeTypeName);

    /// <summary>
    /// Determines whether the specified definition is decorated with an attribute of the named type.
    /// </summary>
    /// <param name="definition">The definition.</param>
    /// <param name="attributeTypeName">Name of the attribute type.</param>
    /// <returns><c>true</c> if the specified definition contains attribute; otherwise, <c>false</c>.</returns>
    public static bool ContainsAttribute(PropertyDefinition definition, string attributeTypeName) =>
        definition.CustomAttributes.Any(a => a.AttributeType.FullName == attributeTypeName);

    /// <summary>
    /// Gets the property names that should be passed to the notification target method when the property value is changed.
    /// </summary>
    /// <param name="propDef">The property definition.</param>
    /// <returns>IEnumerable&lt;System.String&gt;.</returns>
    IEnumerable<string> GetNotifyPropertyNames(PropertyDefinition propDef) {
        //Check for the NotifyAttribute
        var attr = propDef.CustomAttributes.FirstOrDefault(a => a.AttributeType.FullName == NotifyTypeName);

        if (attr != null) {
            //Return property names supplied by the constructor, if none are specified return the property name itself.
            if (attr.HasConstructorArguments) {
                var args = attr.ConstructorArguments[0].Value as CustomAttributeArgument[];
                if (args == null) {
                    //Argument is null
                    yield return null;
                }
                else if (args.Length == 0) {
                    //Apparently the user saw reason to pass an empty array.
                    yield return propDef.Name;
                }
                else {
                    //Multiple arguments have been passed.
                    foreach (var arg in args) {
                        yield return (string)arg.Value;
                    }
                }
            }
            else {
                //No fancy stuff, just return the property name.
                yield return propDef.Name;
            }
        }
    }

    /// <summary>
    /// Weaves notifiers into the property.  This is where the magic happens.
    /// </summary>
    /// <param name="propDef">The property definition.</param>
    /// <param name="notifyTarget">The notify target.</param>
    /// <param name="notifyPropertyNames">The notify property names.</param>
    protected static void InsertNotificationsIntoProperty(PropertyDefinition propDef, MethodReference notifyTarget, IEnumerable<string> notifyPropertyNames) {

        if (propDef.SetMethod == null)
            //This is a read-only property
            return;
        else if (propDef.SetMethod.Body == null) {
            //This is an abstract property, we don't do these either.
            throw new InvalidNotifierException();
        }

        var methodBody = propDef.SetMethod.Body;

        //Retrieve an IL writer
        var msil = methodBody.GetILProcessor();

        //Insert a Nop before the first instruction (like... at the beginning).
        var begin = msil.Create(OpCodes.Nop);
        msil.InsertBefore(methodBody.Instructions[0], begin);

        //Call the notification tareget method for 
        foreach (var notifyPropertyName in notifyPropertyNames) {

            var beginInstructions = new Instruction[0];
            var endInstructions = new Instruction[0];

            //Load the value of the property name to be passed to the notify target onto the stack.
            var propertyName = notifyPropertyName == null ?
                msil.Create(OpCodes.Ldnull) :
                msil.Create(OpCodes.Ldstr, notifyPropertyName);


            //Emit a call to the notify target
            var callNotifyTarget = msil.Create(OpCodes.Call, notifyTarget);
            switch (notifyTarget.Parameters.Count) {
                case 0:
                    endInstructions = new[] {
                        msil.Create(OpCodes.Ldarg_0),
                        msil.Create(OpCodes.Call, notifyTarget),
                        msil.Create(OpCodes.Nop)
                    };
                    break;
                case 1:
                    endInstructions = new[] {
                        msil.Create(OpCodes.Ldarg_0),
                        propertyName,
                        msil.Create(OpCodes.Call, notifyTarget),
                        msil.Create(OpCodes.Nop)
                    };
                    break;
                //This works, but allowing this simply create too many questions.  Eliminating these options in favor of simplicity.
                //In the future I will 
                //case 2:
                //  endInstructions = new[] {
                //      msil.Create(OpCodes.Ldarg_0),
                //      propertyName,
                //      msil.Create(OpCodes.Ldarg_1),
                //      msil.Create(OpCodes.Call, notifyTarget),
                //      msil.Create(OpCodes.Nop)
                //  };
                //  break;
                //case 3:
                //  //this one is a little more complicated
                //  //Create a local variable and set it to the current value of the property.
                //  var variableType = propDef.SetMethod.Parameters[0].ParameterType;
                //  var variableDef = new VariableDefinition($"f__{propDef.Name}_temp", variableType);
                //  propDef.SetMethod.Body.Variables.Add(variableDef);
                //  beginInstructions = new[] {
                //      msil.Create(OpCodes.Ldarg_0),
                //      msil.Create(OpCodes.Call,propDef.GetMethod),
                //      msil.Create(OpCodes.Stloc_0)
                //  };

                //  //Pass propertyname, oldValue and newValue
                //  endInstructions = new[] {
                //      msil.Create(OpCodes.Ldarg_0),
                //      propertyName,
                //      msil.Create(OpCodes.Ldloc_0),
                //      msil.Create(OpCodes.Ldarg_1),
                //      msil.Create(OpCodes.Call, notifyTarget),
                //      msil.Create(OpCodes.Nop)
                //  };
                //  break;
                default:
                    throw new InvalidNotifyTargetException(notifyTarget.FullName);
            }

            var insertionPoint = methodBody.Instructions[methodBody.Instructions.Count - 1];

            //Insert IL instructions before end of method body
            InsertBefore(msil, beginInstructions, begin);
            InsertBefore(msil, endInstructions, insertionPoint);
        }
    }

    protected static void InsertAfter(ILProcessor ilProcessor, IEnumerable<Instruction> instructions, Instruction startPoint) {
        var currentInstruction = startPoint;
        foreach (var instruction in instructions) {
            ilProcessor.InsertAfter(currentInstruction, instruction);
            currentInstruction = instruction;
        }
    }
    protected static void InsertBefore(ILProcessor ilProcessor, IEnumerable<Instruction> instructions, Instruction startPoint) {

        var currentInstruction = null as Instruction;
        foreach (var instruction in instructions) {
            if (currentInstruction == null) {
                ilProcessor.InsertBefore(startPoint, instruction);
            }
            else {
                ilProcessor.InsertAfter(currentInstruction, instruction);
            }
            currentInstruction = instruction;
        }
    }
}
/// <summary>
/// Class NotificationWeaverBuildTask.
/// </summary>
/// <example>
/// place the following XML in the project file.  The directorey containing Mathtone.MIST.Builder.dll should also contain Mathtone.MIST.dll, Mono.Cecil.dll and Mono.Cecil.pdb.dll
/// <UsingTask TaskName = "Mathtone.MIST.NotificationWeaverBuildTask"
///      AssemblyFile="...path to builder assembly"      
/// />
/// <Target Name = "AfterBuild" >
///     <NotificationWeaverBuildTask TargetPath="$(TargetPath)" DebugMode="True"/>
/// </Target>
/// </example>
public class NotificationWeaverBuildTask : Microsoft.Build.Utilities.Task {

    /// <summary>
    /// Gets or sets the target path.
    /// </summary>
    /// <value>The target path.</value>
    [Required]
    public string TargetPath { get; set; }

    /// <summary>
    /// Gets or sets a value indicating whether [debug mode].
    /// </summary>
    /// <value><c>true</c> if [debug mode]; otherwise, <c>false</c>.</value>
    [Required]
    public bool DebugMode { get; set; }

    /// <summary>
    /// When overridden in a derived class, executes the task.
    /// </summary>
    /// <returns>true if the task successfully executed; otherwise, false.</returns>
    public override bool Execute() {
        Log.LogMessage(MessageImportance.High, String.Format(Resources.BuildTaskMessage, TargetPath));
        new NotificationWeaver(TargetPath).InsertNotifications(DebugMode);
        return true;
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ I didn't take time to actually review this yet, but from your description it is awsome - will solve a tone of repetitive work I do all the time! :) \$\endgroup\$ – Mattias Åslund May 28 '16 at 8:41
  • \$\begingroup\$ Ok, now I've tested it and I think it rocks! I especially like how it doesn't trigger notifications if you set the auto-property to the same value it already has. \$\endgroup\$ – Mattias Åslund May 28 '16 at 14:10
  • 1
    \$\begingroup\$ Does such a post build IL modification work with signed assemblies (with a valid signature)? \$\endgroup\$ – JanDotNet May 29 '16 at 9:44
  • \$\begingroup\$ @JNS - Let me get back to you on that. \$\endgroup\$ – Mik3c May 29 '16 at 12:50
  • \$\begingroup\$ @JNS - I get warnings that Mathtone.MIST.dll doesn't have a strong name but it does seem to work fine. \$\endgroup\$ – Mik3c May 29 '16 at 12:53
4
\$\begingroup\$

It looks very interesting. I just find a little bit difficult to read so much code with comments, so I would like to suggest to have even more code but without comments :)

Let’s have a look at NotificationWeaver.GetNotifyPropertyNames(). It has not so many dependencies to the class, just an attribute name which is never used anywhere else. It also requires a lot of comments which have tendency to become outdated.

    /// <summary>
    /// Gets the property names that should be passed to the notification target method when the property value is changed.
    /// </summary>
    /// <param name="propDef">The property definition.</param>
    /// <returns>IEnumerable&lt;System.String&gt;.</returns>
    IEnumerable<string> GetNotifyPropertyNames(PropertyDefinition propDef) {
        //Check for the NotifyAttribute
        var attr = propDef.CustomAttributes.FirstOrDefault(a => a.AttributeType.FullName == NotifyTypeName);

        if (attr != null) {
            //Return property names supplied by the constructor, if none are specified return the property name itself.
            if (attr.HasConstructorArguments) {
                var args = attr.ConstructorArguments[0].Value as CustomAttributeArgument[];
                if (args == null) {
                    //Argument is null
                    yield return null;
                }
                else if (args.Length == 0) {
                    //Apparently the user saw reason to pass an empty array.
                    yield return propDef.Name;
                }
                else {
                    //Multiple arguments have been passed.
                    foreach (var arg in args) {
                        yield return (string)arg.Value;
                    }
                }
            }
            else {
                //No fancy stuff, just return the property name.
                yield return propDef.Name;
            }
        }
    }

It looks good but there are a lot stuff like this in the class. One could argue, but I would rewrite it in a dedicated file SourceNames.cs as:

    class SourceNames : Enumerable<string>
    {
        static readonly string AttributeName = typeof(NotifyAttribute).FullName;

        public SourceNames(PropertyDefinition propertyDefinition)
        {
            PropertyDefinition = propertyDefinition;
        }

        public override IEnumerator<string> GetEnumerator()
        {
            if (!Attributed)
                yield break;

            if (!Customized)
                yield return PropertyName;
            else
                if (NoSources)
                    yield return null;
                else
                    foreach (var source in CustomSources.DefaultIfEmpty(PropertyName))
                        yield return source;
        }

        string PropertyName => PropertyDefinition.Name;
        bool NoSources => CustomSources == null;
        IEnumerable<string> CustomSources => AttributeArguments
            ?.Select(arg => (string)arg.Value);

        IEnumerable<CustomAttributeArgument> AttributeArguments => Attribute
            .ConstructorArguments[0].Value as CustomAttributeArgument[];

        bool Customized => Attribute.HasConstructorArguments;
        bool Attributed => Attribute != null;
        CustomAttribute Attribute => PropertyDefinition.CustomAttributes
            .FirstOrDefault(a => a.AttributeType.FullName == AttributeName);

        PropertyDefinition PropertyDefinition { get; }
    }

Where:

    abstract class Enumerable<T> : IEnumerable<T>
    {
        public abstract IEnumerator<T> GetEnumerator();
        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    }

I would also probably define there:

    static class PropertySourceNames
    {
        public static IEnumerable<string> SourceNames(
            this PropertyDefinition propertyDefinition) =>
            new SourceNames(propertyDefinition);
    }

What do you think?

\$\endgroup\$
  • \$\begingroup\$ Thanks for looking, I commented some of the IL modifying code pretty heavily because without the comments is might not be apparent what the code is doing to someone who hasn't really worked directly with IL before but I could definitely clean up some of the code and extract some of the functionality into other classes. \$\endgroup\$ – Mik3c May 28 '16 at 4:18
  • \$\begingroup\$ Yes, I would consider Composed Method Pattern, which helps a lot with low level manipulations (you can get rid of comments, which always indicate some code smell); SLA to carefully differentiate low level implementation details (like IL – hero only stuff) from the implementation strategy; SRP - the most important one. \$\endgroup\$ – Dmitry Nogin May 28 '16 at 5:07
  • \$\begingroup\$ While I agree with the idea of breaking NotificationWeaver down to smaller parts, personally I find the original code way easier to read and understand. \$\endgroup\$ – Nikita B May 30 '16 at 7:33
3
\$\begingroup\$

A few minor points:

  1. You should consider using С# coding style: { and } always go to the new line.
  2. This looks weird:

    string NotifyTypeName = typeof(NotifyAttribute).FullName;
    

    typeof is a static lookup which is resolved at compilation time, so it does not look like you gain anything by caching the type or it's full name. Are you 100% sure that FullName equality guarantees that types are equal? :) It probably does, at least in your case. But, for example, I would still rather use:

    .FirstOrDefault(a => a.AttributeType.Equals(typeof(NotifyAttribute)));
    

    or even

    .OfType<NotifyAttribute>().FirstOrDefault();
    

    instead of

    .FirstOrDefault(a => a.AttributeType.FullName == NotifierTypeName)
    

    just to be safe.

  3. You don't need to manually Flush the stream before disposing it.

  4. //Apparently the user saw reason to pass an empty array. You should throw an exception, IMHO. If passing empty array does not make any sense, chances are - it's a mistake made by developer. You should not hide it.

  5. I am not a fan of passing debug flags all over the place. Consider using #if DEBUG directive instead.

\$\endgroup\$
  • \$\begingroup\$ First, thanks for your feedback, these are all reasonable suggestions. \$\endgroup\$ – Mik3c May 30 '16 at 11:18
  • 1
    \$\begingroup\$ With regards to code formatting, I understand that the industry has largely adopted a slightly different style of indentation but it just doesn't seem to make sense to drop the opening brace onto a new line all the time, especially if you're using a lot of modern C# features like linq, anonymous types, etc. The war over where that opening brace belongs could choke the mighty Hudson with it's dead so I've always felt that the best approach was to have everyone pick a set of rules that can be enforced by the IDEs auto-formatting capability and whoever is maintaining the code can reformat it. \$\endgroup\$ – Mik3c May 30 '16 at 11:18
  • \$\begingroup\$ The problem with item 2 is that the "AttributeType" is a "TypeReference" from the Mono.Cecil library, I don't think any of the options presented would work. I would much prefer to reference the attribute type in the way you suggest and I tried to make it work this way but using the name was the best way to perform the comparison that I could find. \$\endgroup\$ – Mik3c May 30 '16 at 11:22
  • \$\begingroup\$ Item 3, you're absolutely right I was just flushing the stream out of paranoia and I'll remove that line. \$\endgroup\$ – Mik3c May 30 '16 at 11:22
  • \$\begingroup\$ Regarding item 4 - that was my first instinct too, but I was able to conceive of a few far-fetched scenarios where this might be the desired behavior so the user is able to pass an array of names to be passed to the notification mechanism, if the user explicitly passes an empty array, then I felt like the best thing to do what would be done in any other case, which proceed with notification for all 0 indicated by the user. \$\endgroup\$ – Mik3c May 30 '16 at 11:26
3
\$\begingroup\$

That's looks great, I didn't know that Cecil can be integrated in MSBUILD. To add to another reviewers:

You are using OpCodes.Call, consider using OpCodes.Callvirt, so it will work, if [NotifyTarget] will be virtual/abstract.

In fact C# compiler emits callvirt for each instance call, ass it eforces null-check, but you can emit it only if method is actually virtual.

Additionaly, when looking for [NotifyTarget], search in base type, as potential user can create base class, which will have event itself, and NotifyTarget, and just use notifications in derived classes.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your feedback, the callvirt I will definitely try. If you look near the bottom of the "GetNotifyTarget" method it already searches the chain of inheritance for a target method so yes indeed! You can put the NotifyTarget in a base class. \$\endgroup\$ – Mik3c Jun 2 '16 at 13:02
  • \$\begingroup\$ @Mik3c, oh, sorry I missed that lookup. I guess just replacing with callvirt will do the thing, as overhead is minimal, but generator code will be cleaner than if perform check, if method is actually virtual. P.S. c# usess Call if you need to call base method from virtual one. \$\endgroup\$ – Bogdan Mart Jun 2 '16 at 13:05

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.