12
\$\begingroup\$

There's a problem in Rubberduck that we haven't neatly solved yet, and as we're exploring alternative approaches I figured I'd ask the CR community and see what our C# experts think.

Here's the slightly simplified (irrelevant constructor parameters were removed) constructor for the CodeExplorerViewModel (actual file on GitHub), which is used by the WPF/XAML control for the Code Explorer feature.

The ViewModel needs to expose quite a large number of commands, and since each command has its dependencies, and these dependencies have their own dependencies, we use DI/IoC to constructor-inject the commands into the ViewModel - the DI registration essentially injects every single command instance that exists in Rubberduck all Code Explorer commands1, and then since the XAML command bindings require ICommand properties, we assign these properties by fetching the command of the appropriate type from the List<CommandBase> we're given:

public CodeExplorerViewModel(List<CommandBase> commands)
{
    var reparseCommand = commands.OfType<ReparseCommand>().SingleOrDefault();

    RefreshCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), 
        reparseCommand == null ? (Action<object>)(o => { }) :
        o => reparseCommand.Execute(o),
        o => !IsBusy && reparseCommand != null && reparseCommand.CanExecute(o));

    OpenCommand = commands.OfType<UI.CodeExplorer.Commands.OpenCommand>().SingleOrDefault();
    OpenDesignerCommand = commands.OfType<OpenDesignerCommand>().SingleOrDefault();

    AddTestModuleCommand = commands.OfType<UI.CodeExplorer.Commands.AddTestModuleCommand>().SingleOrDefault();
    AddStdModuleCommand = commands.OfType<AddStdModuleCommand>().SingleOrDefault();
    AddClassModuleCommand = commands.OfType<AddClassModuleCommand>().SingleOrDefault();
    AddUserFormCommand = commands.OfType<AddUserFormCommand>().SingleOrDefault();

    OpenProjectPropertiesCommand = commands.OfType<OpenProjectPropertiesCommand>().SingleOrDefault();
    RenameCommand = commands.OfType<RenameCommand>().SingleOrDefault();
    IndenterCommand = commands.OfType<IndentCommand>().SingleOrDefault();

    FindAllReferencesCommand = commands.OfType<UI.CodeExplorer.Commands.FindAllReferencesCommand>().SingleOrDefault();
    FindAllImplementationsCommand = commands.OfType<UI.CodeExplorer.Commands.FindAllImplementationsCommand>().SingleOrDefault();

    CollapseAllSubnodesCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), ExecuteCollapseNodes);
    ExpandAllSubnodesCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), ExecuteExpandNodes);

    ImportCommand = commands.OfType<ImportCommand>().SingleOrDefault();
    ExportCommand = commands.OfType<ExportCommand>().SingleOrDefault();
    ExportAllCommand = commands.OfType<Rubberduck.UI.Command.ExportAllCommand>().SingleOrDefault();

    _externalRemoveCommand = commands.OfType<RemoveCommand>().SingleOrDefault();
    if (_externalRemoveCommand != null)
    {
        RemoveCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), ExecuteRemoveComand, _externalRemoveCommand.CanExecute);
    }

    PrintCommand = commands.OfType<PrintCommand>().SingleOrDefault();

    CommitCommand = commands.OfType<CommitCommand>().SingleOrDefault();
    UndoCommand = commands.OfType<UndoCommand>().SingleOrDefault();

    CopyResultsCommand = commands.OfType<CopyResultsCommand>().SingleOrDefault();

    SetNameSortCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), param =>
    {
        if ((bool)param == true)
        {
            SortByName = (bool)param;
            SortByCodeOrder = !(bool)param;
        }
    }, param =>
    {
        return SortByName ? false : true;
    });

    SetCodeOrderSortCommand = new DelegateCommand(LogManager.GetCurrentClassLogger(), param =>
    {
        if ((bool)param == true)
        {
            SortByCodeOrder = (bool)param;
            SortByName = !(bool)param;
        };
    }, param => 
    {
        return SortByCodeOrder ? false : true;
    });
}

Some of these commands are used in command bindings for the toolbar, some are used in the context menu, others in link buttons in the bottom panel, and some are reused/redundant, for UX/convenience:

Rubberduck's Code Explorer

Having as many constructor parameters as we need to expose commands would be unacceptable.

Surely there's a better way to do this?

Note: We're currently using , but the IoC is being ported to .


1 [CodeExplorerCommand] attributes decorate the Code Explorer command classes; the IoC configuration is setup so that only command classes decorated with that attribute get injected into the ViewModel.

\$\endgroup\$
  • 1
    \$\begingroup\$ Friend, you know what I'm gonna suggest. Build a Dictionary of the maps and iterate the piss out of that baby. :) \$\endgroup\$ – Der Kommissar Sep 30 '17 at 2:09
  • \$\begingroup\$ @202_accepted I'm not sure I see how a dictionary would help, feel free to post an answer! \$\endgroup\$ – Mathieu Guindon Sep 30 '17 at 2:20
  • \$\begingroup\$ @Mat'sMug I asked similar ( almost same question ) I have a bit different approach there, you can have a look :) codereview.stackexchange.com/questions/176620/… \$\endgroup\$ – Aram Kocharyan Sep 30 '17 at 7:49
  • \$\begingroup\$ @AramKocharyan ah, a service locator! The thought occurred to me I admit, but IMO that's against DI best practices; I'm pretty sure the real, cleanest solution, involves property injection - except Ninject's property injection support requires set-only properties, which is a no-no for me. So the ultimate solution is to ditch Ninject, get on with Castle Windsor, and do proper property injection =) \$\endgroup\$ – Mathieu Guindon Sep 30 '17 at 14:21
  • \$\begingroup\$ @Mat'sMug I agree with you, as long as having large amount (for example 30-40) of properties in ViewModel is acceptable. I was thinking about property injection (before choosing service locator ) , but I didn't know that there is any IoC that supports private set properties. So I guess Castle Windsor supports. I will have a look. \$\endgroup\$ – Aram Kocharyan Sep 30 '17 at 14:46
4
\$\begingroup\$

After rethinking the problem, I got another idea. Because it is fundamentally different, I'll post it in a separate answer.

The commands are injected to the view model, exposed by properties and bound to the view, right? Why not just setting the commands directly in the view?

The following markup extension makes it possible:

public class CommandResolver : MarkupExtension
{
    private readonly Type myCommandType;
    public CommandResolver(Type commandType)
    {
        if (commandType == null)
            throw new ArgumentNullException(nameof(commandType));
        if (!typeof(ICommand).IsAssignableFrom(commandType))
            throw new ArgumentException($"Type '{commandType}' has to implement ICommand.");

        myCommandType = commandType;
    }

    public override object ProvideValue(IServiceProvider serviceProvider)
    {
        var isDesignMode = DesignerProperties.GetIsInDesignMode(new DependencyObject());

        if (!isDesignMode)
        {
            Debug.Assert(Resolve != null, "Resolve must not be null. Please initialize resolving delegate during application startup.");
        }

        return Resolve(myCommandType) ?? DependencyProperty.UnsetValue;
    }

    public static Func<Type, object> Resolve { private get; set; }
}

It must be initialized during applications startup:

var kernel = new StandardKernel();
// ... initialize Ninject kernel (can be used with any other DI container)

CommandResolver.Resolve = type => kernel.Get(type);

The resolver can be used in XAML to probide property types directly:

<Button Command="{resolverns:CommandResolver commandsns:OpenCommand}" />
\$\endgroup\$
  • \$\begingroup\$ Hey that's pretty neat actually! Best option so far I think! \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 20:15
  • \$\begingroup\$ Question: would there be a compile-time error (warning?) if the markup refers to a non-existing command type? \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 20:22
  • \$\begingroup\$ Didn't test it (computer is off now) but I would expect a warning or an error - yes. A downside of this solution is, that a "hidden dependency" will be added to the view. Normally, a view works only with it's view model. \$\endgroup\$ – JanDotNet Oct 2 '17 at 20:29
  • \$\begingroup\$ Yeah, there's that. TBH I think the only clean "by the book" way is to use property injection. But if we're willing to dent the book a little, then this is a beautiful solution. \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 20:36
3
\$\begingroup\$

Ninject has a very nice extension called Ninject.Extensions.Factory. It allows you to create factories by specifying an interface. In your case, you could create a factory that creates the commands:

public interface ICommandFactory
{
    OpenCommand CreateOpenCommand();
    OpenDesignerCommand CreateOpenDesignerCommand();
    // ...
}

Note that dependencies, defined in command's contructors, are automatically injected by the factory extension (Internally, it holds an instance of the kernel).

The factory can be used to create the commands in constructor (or even on the fly if the command is required):

public CodeExplorerViewModel(ICommandFactory commandFactory)
{
    OpenCommand = commandFactory.CreateOpenCommand();
    OpenDesignerCommand = commandFactory.CreateOpenDesignerCommand();
    // ....
}

For me it feels better to use a factory for creating commands compared to registering all commands to the DI container. However, it is more effort to create and maintain the factory interface.

[Edit]

Alternatively, it is possible to create a generic factory for commands. That doesn't require to extend / maintain the interface:

public interface ICommandFactory
{
    TCommand CreateCommand<TCommand>() where TCommand : CommandBase;
}

// ...

public CodeExplorerViewModel(ICommandFactory commandFactory)
{
    OpenCommand = commandFactory.CreateCommand<OpenCommand>();
    OpenDesignerCommand = commandFactory.CreateCommand<OpenDesignerCommand>();
    // ....
}

This solution is simpler to use but not far away from passing the kernel directly. ;).

\$\endgroup\$
  • \$\begingroup\$ Indeed the factory extension is awesome; we use it to bind any *Factory interface to an automagic factory and it's the best thing since sliced bread... However I'm not convinced having an interface that's literally designed to change is a good idea, even though the implementation is provided on the fly by the IoC container: there must be a way to do this and agree with SOLID principles! That said, how are the command types configured? Bound .ToSelf()? \$\endgroup\$ – Mathieu Guindon Oct 1 '17 at 18:52
  • \$\begingroup\$ @Mat'sMug: Please see my updated answer. The second solution avoids the "interface that's literally designed to change". Not sure if I understand the question correctly, but it is not necessary to register the commands to the kernel. The factory creates new command instances on the fly. \$\endgroup\$ – JanDotNet Oct 2 '17 at 8:53
1
\$\begingroup\$

I would typically create a manager for this. Make a class that contains all the properties for each command. Not a factory because we will still allow the DI container to create the commands we just making a nicer way to access them.

Now I don't want to keep updating the manager each time a new command is created. Which is why the t4 templates were created. We can make a template that reads the command directory and creates a property for each command and assigns that value in the constructor.

I created a simple t4 template called CommandManager.tt

<#@ template debug="false" hostspecific="true" language="C#" #>
<#@ assembly name="System.Core" #>
<#@ import namespace="System.IO" #>
<#@ import namespace="System.Linq" #>
<#@ import namespace="System.Text" #>
<#@ import namespace="System.Collections.Generic" #>
<#@ output extension=".cs" #>

<#
   // whatever namespace you want.  If you want to read namespace hints there are stack overflow examples this is just a simple example
   var namespaceName = "Rubberduck.UI.CodeExplorer.Commands";
   var t4 = Host.TemplateFile;
   // Realtive path to commands from t4 template
   var path = Path.Combine(Path.GetDirectoryName(t4), @"..\..\..\Commands");
   var commands = Directory.GetFiles(path, "*Command.cs").Select(Path.GetFileNameWithoutExtension).ToArray();
#>

using System.Collections.Generic;
using System.Linq;
using System.Windows.Input;

namespace <#= namespaceName #>
{
    public class <#= Path.GetFileNameWithoutExtension(t4) #>
    {
        public <#= Path.GetFileNameWithoutExtension(t4) #>(IList<ICommand> commands)
        {
<#
   foreach (var command in commands)
   {
#>
           <#=command #> = commands.OfType<<#=namespaceName #>.<#=command #>>().SingleOrDefault();
<#
   }
#>
        }

<#
   foreach (var command in commands)
   {
#>
        public ICommand <#=command #> {get; }
<#
   }
#>
    }
}

I would register this class as a singleton and I made the properties read-only. This will create a class something like this

using System.Collections.Generic;
using System.Linq;
using System.Windows.Input;

namespace Rubberduck.UI.CodeExplorer.Commands
{
    public class CommandManager
    {
        public CommandManager(IList<ICommand> commands)
        {
           AddClassModuleCommand = commands.OfType<Rubberduck.UI.CodeExplorer.Commands.AddClassModuleCommand>().SingleOrDefault();
           AddStdModuleCommand = commands.OfType<Rubberduck.UI.CodeExplorer.Commands.AddStdModuleCommand>().SingleOrDefault();
           ExportCommand = commands.OfType<Rubberduck.UI.CodeExplorer.Commands.ExportCommand>().SingleOrDefault();
           PrintCommand = commands.OfType<Rubberduck.UI.CodeExplorer.Commands.PrintCommand>().SingleOrDefault();
        }

        public ICommand AddClassModuleCommand {get; }
        public ICommand AddStdModuleCommand {get; }
        public ICommand ExportCommand {get; }
        public ICommand PrintCommand {get; }
    }
}

Notes about this. This reads the file name and assumes the class is the same as the file. If not the linq shouldn't fail the command will just be null. Up to you if you want it to throw at runtime so you know you have an issue or not.

Also I made the class partial so if there is anything special that you need added that doesn't have to be in the t4 template you can create another partial class and add it there.

Now I would inject the CommandManger class into your ViewModel and set it's property from properties from the CommandManager. This class now is reusable in all your viewmodels and each one doesn't have to iterate over the list of commands to find the one it needs.

\$\endgroup\$
  • 2
    \$\begingroup\$ In other words, move the ugliness into another class, and have the redundant LINQ code generated by a t4 template. Interesting approach, but IMO just moves the problem elsewhere - that generated class now has the constructor injection and command sorting-out responsibility, and the CommandManager becomes some sort of command locator. I'd even name it as such. \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 15:47
  • \$\begingroup\$ Exactly. Unfortunately DI containers don't have a good way to know the difference between classes with the same interface. Unity has a string you can pass in to it to resolve a specific type but again I wouldn't want that in my VM and would prefer to keep ugly code in one spot and not have all my WM have ugly code. Maybe one day there can be a better solution and then you just need to replace one class an not change all the VM. Even the other answer is just like a kernel resolver as well. \$\endgroup\$ – CharlesNRice Oct 2 '17 at 15:52
  • \$\begingroup\$ The other option is to constructor inject all the commands in the t4 template and assign them instead of using linq. If that makes it more palatable to you. Since it's a t4 template it would do the hard work of adding all the classes into the constructor. \$\endgroup\$ – CharlesNRice Oct 2 '17 at 15:53
  • \$\begingroup\$ I'm seriously considering putting that cleanup on hold until Castle Windsor port is finished, and then go with full-on property injection using a naming convention on the command properties; that way I remove all constructor injection, have no need for attributes, the properties can be exposed to XAML as the ICommand interface they need to be, and the only problem becomes documenting how these magic properties get assigned. \$\endgroup\$ – Mathieu Guindon Oct 2 '17 at 15:58
1
\$\begingroup\$

After reading all discussions/answers here I come to this solution. To be clear I just adopted @JanDotNet 's solution for ViewModel. So let's start.

First, we don't want to have huge amount of useless command-properties declared. So we need to archive typesafe access from View or ViewModel (As I see there is similar case in Rubberduck) without declaring properties.In second, we won't having external dependencies for our view. Third, have you seen this mem ?

standards

I want to say that this solution isn't intended to be the best. I just want to share my solution. ( I had similar problem. )

CommandBindings
In this the data structure we will store our commands.

public class CommandBindings
{
    public CommandBindings(IEnumerable<ICommand> commands)
    {
        _underlyingCommands = commands ?? throw new ArgumentNullException(nameof(commands));
    }

    public ICommand OfType(Type type)
    {
        return _underlyingCommands.FirstOrDefault(x => x.GetType() == type);
    }

    private IEnumerable<ICommand> _underlyingCommands;
}

CommandBindingsProvider
This service will provide our CommandBindings, by retrieving commands with appropriate attributes. ( or however it is necessary )

public class CommandBindingsProvider : ICommandBindingsProvider
{
    public static Func<Type, object> Resolve { private get; set; }

    public CommandBindings GetBindings()
    {
        if (Resolve == null)
            throw new InvalidOperationException("CommandBindngs Resolve must be set in application startup.");

        var commands = new List<ICommand>();

        var commandTypes = GetCommandTypesByAttribute();
        foreach (var commandType in commandTypes)
        {
            var command = Resolve(commandType) as ICommand;
            Debug.Assert(command != null, $"Command {commandType.Name} isn't registered in container.");
            if (command != null)
            {
                commands.Add(command);
            }
        }

        var bindings = new CommandBindings(commands);
        return bindings;
    }

    private static IEnumerable<Type> GetCommandTypesByAttribute()
    {
        ...
    }
}

CommandBindingsProvider.Resolve must be initialized during application startup:

var kernel = new StandardKernel();
// ... initialize Ninject kernel (can be used with any other DI container)  
CommandBindingsProvider.Resolve = type => kernel.Get(type);

IVMWithCommandBindings
ViewModel that has commands with CommandBindings will implement this interface.

public interface IVMWithCommandBindings
{
    CommandBindings CommandBindings { get; }
}

Now our ViewModel:

public class ExplorerVM : IVMWithCommandBindings
{
    public ExplorerVM(ICommandBindingsProvider commandBindingsProvider)
    {
        CommandBindings = commandBindingsProvider.GetBindings();
    }

    public CommandBindings CommandBindings { get; }
}

We have our ViewModel with CommandBindings. Now we must define a way of using this CommandBindings in XAML. Markup extension comes here:

CommandResolver

public class CommandResolver : MarkupExtension
{
    public CommandResolver(Type commandType)
    {
        if (commandType == null)
            throw new ArgumentNullException(nameof(commandType));
        if (!typeof(ICommand).IsAssignableFrom(commandType))
            throw new ArgumentException($"Type '{commandType}' has to implement ICommand.");

        myCommandType = commandType;
    }

    public override object ProvideValue(IServiceProvider serviceProvider)
    {
        var tp = (IProvideValueTarget)serviceProvider.GetService(typeof(IProvideValueTarget));
        if (tp == null) return DependencyProperty.UnsetValue;

        var target = tp.TargetObject as FrameworkElement;
        if (target == null) return DependencyProperty.UnsetValue;

        var vmWithCommandBindings = target.DataContext as IVMWithCommandBindings;
        if (vmWithCommandBindings == null)
            throw new InvalidOperationException("CommandResolver is designed to be use with IVMWithCommandBindings data contexts.");

        return vmWithCommandBindings.CommandBindings.OfType(myCommandType) ?? DependencyProperty.UnsetValue;
    }

    private readonly Type myCommandType;
}

Here we retrieve element's DataContext in which markup extension is used. Then we check that DataContext object implements IVMWithCommandBindings interface, after that we resolve appropriate command with passed type.

Finally:

Usage in XAML:

<Button Command="{resolvers:CommandResolver commands:TestCommand}"/>

Usage in ViewModel:

CommandBindings.OfType(typeof(TestCommand)).Execute(null);

Looks ugly, but if we write Extension for IVMWithCommandBindings interface like this:

public static class VMWithCommandBindingsExtensions
{
    public static void ExecuteCommand(this IVMWithCommandBindings vm, Type type, object param = null)
    {
        // skipping null checks for brevity sake 
        var command = vm.CommandBindings.OfType(type);
        if (command.CanExecute(param))
        {
            command.Execute(param);
        }
    }
}

we will have Usage in ViewModel like:

this.ExecuteCommand(typeof(TestCommand));
\$\endgroup\$

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.