6
\$\begingroup\$

I am implementing the command pattern in a project that I am working on and I have an interface, ICommandFactory that all of my commands are implementing.

When I run the application I want to dynamically gather all classes in the current assembly that implement that interface, thus producing a list of all possible commands.

So I came up with the method below. This was the first thing that I could get to work. Is there a better way to do this?

    private static IEnumerable<ICommandFactory> GetAvailableCommands()
    {
        var interfaceType = typeof (ICommandFactory);
        var implementors = Assembly.GetExecutingAssembly().GetTypes().Where(
            i => interfaceType.IsAssignableFrom(i) && i != interfaceType);

        var commands = new List<ICommandFactory>();

        foreach(var implementor in implementors)
        {
            var ctor = implementor.GetConstructor(Type.EmptyTypes);
            if (ctor == null) continue;
            var instance = ctor.Invoke(null) as ICommandFactory;
            if (instance == null) continue;
            commands.Add(instance);
        }

        return commands;
    }
\$\endgroup\$
2
\$\begingroup\$

Your code seems to do what it is supposed to do.

I'm worried about the fact that you're hinting at the possibility of having multiple CommandFactory classes. Why would you want that? (I'm just asking. You might have very valid reasons.)

I'm also wondering if you actually meant Factory, since the variable name you use for the list inside your method is just commands, which makes me believe that you are looking for just that: types that implement ICommand, not ICommandFactory.

\$\endgroup\$
  • \$\begingroup\$ Lette - You are right, Factory is a bad naming convention. These classes implement both the ICommandFactory and ICommand interface. Where the ICommandFactory has methods that are used to build a ICommand that is implemented. Probably renaming to something like, ICommandBuilder might be better. I really appreciate you reviewing this method for me. \$\endgroup\$ – Paige Cook Feb 23 '11 at 1:55
5
\$\begingroup\$

I only have a few minor comments about your code:

  1. This code:

    var ctor = implementor.GetConstructor(Type.EmptyTypes);
    if (ctor == null) continue;
    

    has the potential to mask a bug. Imagine you add a constructor to one of your command-factory types and accidentally forget that this implicitly removes the default constructor. This code will then silently swallow this mistake and simply not list that particular command factory. It would be preferable to throw in this case so that you notice it immediately. Even better, of course, would be to use a post-build event to turn this error into a compile-time check so that you can’t even run the program when it is invalid.

  2. This code:

    var instance = ctor.Invoke(null) as ICommandFactory;
    if (instance == null) continue;
    

    has exactly the same problem, but the consequences are much more subtle. The if (instance == null) condition should never fire because the code was written so that the instance cannot be of any other type. Imagine you have a bug in that code. This code swallows this bug and you won’t notice it directly. You will only notice some subtle/weird effects that occur much later, and it will be a pain to trace it back to this code here. You could make it throw like above, but personally I think you should change this to var instance = (ICommandFactory) ctor.Invoke(null); so that it will automatically throw if it’s not of the right type.

  3. Your code does not allow for a class that implements ICommandFactory but is not intended to appear in your UI. If you are sure that you don’t want that to be possible, then your code is fine. Otherwise, I would declare a custom attribute — either one that allows you to explicitly exclude an ICommandFactory from appearing in the UI, or one that is required on all the ICommandFactory types to be included.

  4. Very very minor nitpick:

    i => interfaceType.IsAssignableFrom(i) && i != interfaceType
    

    You should perform the simpler check first:

    i => i != interfaceType && interfaceType.IsAssignableFrom(i)
    
\$\endgroup\$
  • \$\begingroup\$ Thanks @Timwi for your comments. I had not even realized that I would be swallowing the mistakes in points 1 & 2 above. \$\endgroup\$ – Paige Cook Feb 25 '11 at 12:54
0
\$\begingroup\$

You can use the Activator to create an instance:

commands.Add( Activator.CreateInstance(implementor) as ICommandFactory );

or even a fancy Compiled Lamdba Expression.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback. I will look into the Compliled Lambada Expressions as they are intriguing. \$\endgroup\$ – Paige Cook Feb 23 '11 at 17:04
  • \$\begingroup\$ You don’t seriously think that using a compiled lambda expression would be better in this case? It is harder to read, less maintainable, and probably also slower. (The graph in your link only shows the invocation time, not the construction time, but in this example it is invoked only once...) \$\endgroup\$ – Timwi Feb 25 '11 at 17:24

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.