How to seperate commands out of big viewmodel

Question

My viewmodel contains lots of commands, it makes my viewmodel very big. I want to seperate my command out of the viewmodel. Currently, My soluton is create a class for each command like below,

 public class TestCommand : CommandBase
{
    private MainViewModel vm;

    public TestCommand(MainViewModel vm)
    {
        this.vm = vm;
    }

    public override bool CanExecute(object parameter)
    {
        return true;
    }

    public override void ExecuteCommand(object parameter)
    {
        vm.logger.log(...);
        ...
    }
}

Since I need to use some method or properties in ViewModel, so I have to pass the viewmodel as parameter to the command. For this solution, there're two disadvantages: 1. There're lots of command files in the project, if the average count of the command in one view is 15, 10 views will have 150 command files in the project; 2. Passing ViewModel as parameter to command needs some properties or methods which should be private have to been change to public; Also it's very strange to pass viewmodel to command.

Is there any other solution to seperate commands?

Answer 1

TL;DR:

ViewModels are presentation logic which is expressed mostly in commands, so it's not unusual that the commands take a big chunk of ViewModel's code. Don't try too hard to make the ViewModels as plain data holders (like common in ASP.NET MVC when using ViewModels) with INotifyPropertyChanged.

Long version

With the lack of more details it's hard to give you concrete tips, but here are some general guidelines. You may update your question with more details about the kind of commands you are using and I'll try to update the question.

1. Presentation Logic

   Main concern of ViewModels is presentation. There is no place for business Logic in the ViewModels.

   Business logic has to be extracted to either your business/domain models (if you follow the rich domain model) or services (in anemic domain model). In rich domain model your service layers are usually quite thin and mostly there to coordinate actions between multiple models.

   So if your ViewModel/commands are doing any kind of logic not related to presentation (if click button A, disable Buttons B, C and D or hide GroupBoxA or "disable button A if data is missing (CanExecute of ICommand)) it's probably doing too much.

2. Separation of concerns:

   It may be possible that your ViewModel tries to do more than it's intended to do. The logger in your example is such a hint. Logging is not a ViewModel concern.

   ViewModel is about presentation and presentation logic and logging is an application concern (as it do not belong to domain/business logic).

   Often a ViewModel can be split into two or more ViewModels (i.e. A ViewModel that manages a list of Customers and allow to edit the selected customer, usually can be split into 2 or 3 ViewModels: CustomersViewModel (display list), CustomerDetailViewModel or CustomerViewModel (detail to the customer) and a CustomerEditViewModel (to edit the customer in question)

   Concerns like logging and caching should be done using i.e. Decorator pattern. This requires that your services and/or repositories properly use and implement interfaces, then you can create decorators for caching or logging and rather than inject the original instance of your service, you implement the decorator.

   Dependency Injection (DI) and Inversion of Control (IoC) containers really help you with this. Having to manually wire this up (aka poor mans DI) is pain in the butt. Concrete examples are out of the scope of this answer.

3. Business Logic in Commands

   Commands shouldn't contain business logic. When your commands contain too much code (usually more than 5-20 lines of code) this is a good clue, that your commands may do too much.

   Commands should really only wire up multiple calls to services and assign data to Properties and/or rise events/messages (that are relevant to the presentation layer. Not to be confused with domain events, which shouldn't be raised inside commands). They are similar to "Actions" in MVC (like used frameworks in ASP.NET MVC for example).

   Commands should usually look something like this

   var customer = new Customer { Name = this.CustomerName, Mail = this.CustomerMail };
   try {
       this.customerService.AddCustomer(customer);
       // Add it to Observable<Customer> list so the UI gets updated
       this.Customers.Add(customer);
       // the service should have populated the Id field of Customer when persisting it
       // so we notify all other ViewModels that a new customer has been added
       this.messageBus.Publish(new CustomerCreated() { CustomerId = customer.Id } );
   } catch (SomeSpecificException e) { // Handle the Exception } 
   

   or

   this.Customers = this.customerRepository.GetAll();
   // Or this for async commands
   this.Customers = await this.customerRepository.GetAllAsync();
   

4. Encapsulation

   Many commands are very tightly coupled to the ViewModel itself and need access to internal state of the the ViewModel or the Model (the Model shouldn't be directly exposed to the View, this couples the Model to the View and any change in the model breaks your View and bindings).

   Moving these ICommands out the ViewModels may be difficult without breaking encapsulation.

Of course you can also implement multiple commands in one class

public class MyViewModelCommandHandler
{
    private readonly IMyRepository myRepository;

    public MyViewModelCommandHandler(/* pass dependencies here*/)
    {
        // assign and guard dependencies

        MyCommand = new RelayCommand(MyCommand, CanExecuteMyCommand);
        MyOtherCommand = new RelayCommand(MyOtherCommand, CanExecuteMyOtherCommand);
    }

    public ICommand MyCommand { get; protected set; } 
    public ICommand MyOtherCommand { get; protected set; } 

    private void MyCommand() 
    {
        // do something
    }

    private void CanExecuteMyCommand() 
    {
        // validate
    }

    private void MyOtherCommand() 
    {
        // do something else
    }

    private void CanExecuteMyOtherCommand() 
    {
        // validate
    }
}

And in your ViewModel simply assign these commands

public class MyViewModel : ViewModelBase 
{
    public MyViewModel()
    {
        var commandHandler = new MyCommandHandler(this);
        OneCommand = commandHandler.MyCommand;
        OtherCommand = commandHandler.MyOtherCommand;
    }

    public ICommand OneCommand { get; private set; } 
    public ICommand OtherCommand { get; private set; } 
}

You can also inject your MyCommandHandler into your view using an IoC container, this require remodeling your command handler class a bit, to create the ICommand on demand. Then you may use it like

public class MyViewModel : ViewModelBase 
{
    public MyViewModel(MyCommandHandler commandHandler)
    {
        OneCommand = commandHandler.CreateMyCommand(this);
        OtherCommand = commandHandler.CreateMyOtherCommand(this);
    }

    public ICommand OneCommand { get; private set; } 
    public ICommand OtherCommand { get; private set; } 
}

But that just shifts your problem, it won't solve the points 1. to 5. though. So I'd suggest to try the suggestions from the aboves list first and if your commands still contain "too many lines of code", try the other solution.

I don't like it too much as it creates unnecessary abstractions for little gain.

It's not uncommon that the ViewModels mainly consist of presentation logic, as that's their purpose and presentation logic is typically inside commands. Except that, you just have properties and the constructor. Properties shouldn't have anything else other than checking if value changed, then assign and one or more OnPropertyChanged calls.

So 50-80% of your ViewModel is code from commands.

Answer 2

Check if your view model can be divided into logical blocks and create sub-viewmodels for each blocks. The extra advantage is that these smaller viewmodels can be often reused when you want to display the same information in a different way somewhere else.

Also I prefer to have a general RelayCommand definition and just creating the commands in my viewmodel without specifying the different methods so I can keep the Execute and CanExecute together as lambda-expressions.

If creating different viewmodels isn't possible you can also split the code of your class over multiple files (partial classes) to increase maintainability.

Answer 3

Answer to your question is Single Responsibility Principle. Your viewmodel is doing too much. Separate the functionality from your vm and put it into different classes and send the classes as reference to your command. In your case

public class TestCommand : CommandBase
{
    private Logger logger;

    public TestCommand(Logger logger)
    {
        this.logger = logger;
    }

    public override bool CanExecute(object parameter)
    {
        return true;
    }

    public override void ExecuteCommand(object parameter)
    {
        logger.log(...);
    }
}

Here I have sent the Logger object to the Command instead of the view model. Also having lot of command files in a project is a good practice only, as long as you keep them in a logical folder.

Note : In Real world we don't do only logging on a command execution. basically we do some functionality and log the same. The only reason i have used logger here is only because for the OP's quick understanding. Ideally we should send a class which has the functionality which has to be done on command execution.

Answer 4

Using ICommand as a message pattern

This solution focuses on Separation of Concerns and the Single Responsibility Principle

It allows you to skip the RelayCommand Pattern in MVVM.

If you are using XAML you can refer to the namespace that has the single command class. Like this:

 xmlns:cmd="clr-namespace:MyProject"

Then a global or local style can be defined as shown here. This makes all buttons use just one command passing in the text of the button as parameter. Most buttons use Text as context but the Tag could be used too.

        <Style BasedOn="{StaticResource XDButton}" TargetType="{x:Type Button}">
            <Setter Property="Command" Value="{StaticResource ResourceKey=cmd}"/>
            <Setter Property="CommandParameter" Value="{Binding Content, RelativeSource={RelativeSource Self}}"/>
        </Style>

You can create just one command for entire project like this, notice the 'routing' is based on the button text. 'Favor naming conventions over configuration'

   public class Commands : ICommand
    {
        private bool canExecute = true;

        public bool CanExecute(object parameter)
        {
            return canExecute;
        }

        public event EventHandler CanExecuteChanged;

        public void Execute(object parameter)
        {
            NotifyCanExecute(false);
            var information = parameter.ToString();
            try
            {
                if (information == "Show Passed") Events.ShowAllPassedTests(this, new EventArgs());
                if (information == "Show Failed") Events.ShowAllFailedTests(this, new EventArgs());
                if (information == "Sort By elapsed Time") Events.SortByElapsedTime(this, new EventArgs());
                if (information == "Sort By Run Data") Events.SortByRunData(this, new EventArgs());
                if (information == "Sort By Title") Events.SortByTitle(this, new EventArgs());
                if (information == "Generate HTML Report") Events.GenerateHTMLReport(this, new EventArgs());
            }
            catch (NullReferenceException nre) {
                Trace.WriteLine("Test Runner Commands 320- An attempt to fire an event failed due to no subscribers");
            }
            NotifyCanExecute(true);
        }

        private void NotifyCanExecute(bool p)
        {
            canExecute = p;
            if (CanExecuteChanged != null) CanExecuteChanged(this, new EventArgs());
        }
    }

Create a single Events aggregation class like this:

public  class Events  
{
    public static EventHandler ShowAllPassedTests;
    public static EventHandler ShowAllFailedTests;
    public static EventHandler ClearAllFilters;
    public static EventHandler SortByElapsedTime;
    public static EventHandler SortByRunData;
    public static EventHandler SortByTitle;
    public static EventHandler GenerateHTMLReport;
    public static EventHandler<CheckBox> ColumnViewChanged;
}

You can create a decoupled Navigator user control with buttons in it like this. When the button is clicked it just calls the Command class passing in the Button context.

 <StackPanel Orientation="Vertical">
        <StackPanel.Resources>
            <Style BasedOn="{StaticResource XDButton}" TargetType="{x:Type Button}">
                <Setter Property="Command" Value="{StaticResource ResourceKey=cmd}"/>
                <Setter Property="CommandParameter" Value="{Binding Content, RelativeSource={RelativeSource Self}}"/>
            </Style>
        </StackPanel.Resources>
        <Button x:Name="XBTNShowPassed"  >Show Passed</Button>
        <Button x:Name="XBTNShowFailed"  >Show Failed</Button>
        <Button x:Name="XBTNShowAll"  >Show All</Button>
        <Button x:Name="XBTNSortByElapsedTime"  >Sort by Elapsed Time</Button>
        <Button x:Name="XBTNSortByRunData"  >Sort By Run Data</Button>
        <Button x:Name="XBTNSortByTitle"  >Sort By Title</Button>
        <Button x:Name="XBTNGenerateHTMLReport"  >Generate HTML Report</Button>
    </StackPanel>

Finally the receiving ViewModel or other class looks like this:

            Events.ColumnViewChanged += OnColumnViewChanged;
            Events.SortByTitle += OnSortByTitle;
            Events.SortByRunData += OnSortByRunData;
            Events.SortByElapsedTime += OnSortByElapsedTime;
            Events.GenerateHTMLReport += OnGenerateHTMLReport;
            Events.ShowAllFailedTests += OnShowAllFailedTests;
            Events.ShowAllPassedTests += OnShowAllPassedTests;

        }

        private void OnShowAllPassedTests(object sender, EventArgs e)
        {
            FilterCVS(tr => tr.DidTestPass);
        }

        private void OnShowAllFailedTests(object sender, EventArgs e)
        {
            FilterCVS(tr => tr.DidTestFail);
        }

Don't forget to implement Dispose

When code hooks up to an EventHandler it becomes ineligible for Garbage Collection. To fix this, implement the Dispose pattern and disconnect the eventhandlers... e.g

Events.OnColumnViewChanged -= OnColumnViewChanged;