Collectives™ on Stack Overflow

Find centralized, trusted content and collaborate around the technologies you use most.

Learn more about Collectives

Teams

Q&A for work

Connect and share knowledge within a single location that is structured and easy to search.

Learn more about Teams

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?

possible duplicate of Should I check an ICommand's CanExecute method before calling Execute from procedural code? – CodeNaked Aug 8, 2011 at 15:25 @CodeNaked: This doesn't strike me as a duplicate of that. This asks about calling CanExecute() within Execute(), the other asks about calling it outside of the implementation. – Joel B Fant Aug 8, 2011 at 15:30 Yeah, I saw that question. I am more concerned here with a consistent Implementation of the ICommand interface internals, and less concerned with HOW it is called. – Sheldon Warkentin Aug 8, 2011 at 15:39

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.

sometimes there is a perfectly valid reason to bypass the Can execute test, say the user doesn't have permission to call the function directly but a background task can run it on the users behalf. so i would say unless the calling the Execute when CanExecute is false would be actively harmful to the systems then calling canExecute is limiting, however if it is actively harmful then there is no valid reason to bypass and the calling the canExecute should be enforced, though i would probably do this via caching the result and checking that rather than calling the function again – MikeT Apr 9, 2018 at 13:01

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.

CanExecute should be designed to be as quick as possible as it is called frequently by the binding framework. – Siy Williams Aug 8, 2011 at 15:36 I did run into this scenario. The CanExecute was taking a fairly long time, and calling it twice did not improve this. A workaround was to watch for property changes on the source, and then keep a single 'CanExecute' variable -- though this quickly became unmanageable because 'CanExecute' can be called in completely different context and timing than the property change code. – Sheldon Warkentin Aug 8, 2011 at 15:36

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); 
                Sorry, I wasn't clear in my question that this was the WPF ICommand interface that I am working with.
– Sheldon Warkentin
                Aug 8, 2011 at 15:54
                @Sheldon ah! thanks for the info.  In that case I would probably use a marker to indicate whether it's been checked or not. I'll append my answer.
– Joseph
                Aug 8, 2011 at 15:59

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.

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.

I think the exception might be a good thing as it possibly points to flawed code where Execute was called manually without doing any checking first (maybe expecting that it will execute for sure). – H.B. Aug 8, 2011 at 15:32 To me, it depends on what the ICommand is going to do. I've had actions that should throw if called manually, and others that are ok to silently pass by. I'm going to edit and expound on that. – Joel B Fant Aug 8, 2011 at 15:36

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.

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

Thanks for contributing an answer to Stack Overflow!

  • Please be sure to answer the question. Provide details and share your research!

But avoid

  • Asking for help, clarification, or responding to other answers.
  • Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.