CODEWITHSUNDEEP
c#wpfdesign-patternscommand
Sheldon Warkentin
Sheldon Warkentin

Reputation: 1736

ICommand - Should I call CanExecute in Execute?

Given that System.Windows.Input.ICommand as 2 primary methods:

interface ICommand {
  void Execute(object parameters);
  bool CanExecute(object parameters);
  ...
}

I expect CanExecute(...) to be called in the Command-supported frameworks before Execute(...) is called.

Internally to my Command Implementation, however, is there any reason to add the CanExecute(...) call inside my Execute(...) implementation?

e.g.:

public void Execute(object parameters){
  if(!CanExecute(parameters)) throw new ApplicationException("...");
  /** Execute implementation **/
}

This becomes relevant in my testing, as I may mock out some interfaces to support CanExecute, and have to do the same mocks when testing Execute.

Any design thoughts on this?

Upvotes: 17

Views: 6672

Answers (7)

LoRdPMN
LoRdPMN

Reputation: 532

I know this is an old question, but for reference purposes, you could implement a SafeExecute method inside your generic ICommand implementation, to not have to repeatedly call CanExecute every time you need to manually execute, like this:

public void SafeExecute(object parameter) {
    if (CanExecute(parameter)) {
        Execute(parameter);
    }
}

Of course this will not prevent from calling directly Execute(), but at least you follow DRY concept and avoid calling it twice every execution.

The same could be implemented to throwing an exception on CanExecute() returning false.

Upvotes: 2

Joy George Kunjikkuru
Joy George Kunjikkuru

Reputation: 1528

CanExecute is MSFT's easy way(I would say hack) to integrate command with the UI controls such as button. If we associate a command with a button, FWK can call the CanExecute and enable/disable the button. In that sense we should not be calling CanExcute from our Execute method.

But if we think from OOP / reusable perspective we can see that ICommand can be used for non-UI purpose as well such as an orchestration / automation code calling my Command. In that scenario it is not necessary that automation will call the CanExecute before calling my Execute().So if we want to have our ICommand implementation working consistently, we need to call CanExecute(). Yes there is performance problem and we have to adjust with it.

As per my view, the .Net framework has violated the ISP in this command, like how its violated for ASP.Net Membership provider.Please correct if I am wrong

Upvotes: 0

Siy Williams
Siy Williams

Reputation: 2446

Programmers are notoriously lazy and they will call Execute without calling CanExecute first.

The ICommand interface is more often used with the WPF binding framework however it is an extremely robust and useful pattern that can be used elsewhere.

I call CanExecute immediately from the Execute method to validate the state of the object. It helps reduce duplicate logic and enforces the use of the CanExecute method (why go through all the effort of whether they can call a method and not bother enforcing it?). I don't see a problem with calling CanExecute lots of times because it should be a quick operation anyway.

I do however always document the results of calling an Execute method if the CanExecute method returns false, so that the consumer knows the consequences.

Upvotes: 9

Joseph
Joseph

Reputation: 25523

I would go one way or the other, but not both.

If you expect the user to call CanExecute, then don't call it in Execute. You've already set that expectation in your interface and now you have a contract with all the developers that implies this sort of interaction with ICommand.

However, if you're worried about developers not utilizing it properly (as you could rightfully be), then I would suggest removing it from the interface completely, and making it an implementation concern.

Example:

interface Command {
    void Execute(object parameters);    
}

class CommandImpl: ICommand {
    public void Execute(object parameters){
        if(!CanExecute(parameters)) throw new ApplicationException("...");
        /** Execute implementation **/
    }
    private bool CanExecute(object parameters){
        //do your check here
    }
}

This way, you're contract (interface) is clear and concise, and you won't be confused about whether or not CanExecute is getting called twice.

However, if you're actually stuck with the interface because you don't control it, another solution could be to store the results and check it like this:

interface Command {
    void Execute(object parameters);    
    bool CanExecute(object parameters);
}

class CommandImpl: ICommand {

    private IDictionary<object, bool> ParametersChecked {get; set;}

    public void Execute(object parameters){
        if(!CanExecute(parameters)) throw new ApplicationException("...");
        /** Execute implementation **/
    }

    public bool CanExecute(object parameters){
        if (ParametersChecked.ContainsKey(parameters))
            return ParametersChecked[parameters];
        var result = ... // your check here

        //method to check the store and add or replace if necessary
        AddResultsToParametersChecked(parameters, result); 
    }
}

Upvotes: 3

Joel B Fant
Joel B Fant

Reputation: 24766

It will probably cause no harm to call CanExecute() within Execute(). Normally Execute() is prevented if CanExecute() would return false since they are usually bound to the UI and not called in your own code. However, nothing forces someone to manually check CanExecute() before calling Execute(), so it's not a bad idea to embed that check.

Some MVVM frameworks do indeed check it before invoking Execute(). They don't throw an exception, though. They simply don't call Execute(), so you may not want to throw an exception yourself.

You might consider basing whether you throw an exception if CanExecute() returns false on what Execute() would do. If it would do things that would be expected to complete by anything following the call to Execute(), then throwing makes sense. If the effects of calling Execute() are not so consequential, then maybe silently returning is more appropriate.

Upvotes: 1

Michael Sagalovich
Michael Sagalovich

Reputation: 2549

I would not be as optimistic as others about adding a call to CanExecute into Execute implementation. What if your CanExecute execution takes a very long time to complete? This would mean that in real-life your user will wait twice that long - once when CanExecute is called by environment, and then when it is called by you.

You could possibly add some flags to check whether CanExecute has already been called, but be careful to keep them always up to command state not to miss or perform unwanted CanExecute call when the state has changed.

Upvotes: 3

David Masters
David Masters

Reputation: 8295

I don't see a problem with adding it. As you say, normally the framework will call the CanExecute before the Execute (making a button invisible for example) but a developer may decide to invoke the Execute method for some reason - adding the check will provide a meaningful exception if they do so when they shouldn't.

Upvotes: 1

Related Questions