0

I think that there is a specific answer to this.

If I have a command binding 

private bool CanExecute(Object args){
  // Should this just be null checks?
  // Should it also contain logic? 
  // example:

  return this.SelectedObject != null;

  // or 

  return this.SelectedObject != null && this.SelectedObject.Status == 1;
}

private void Executed(Object args){

  //Or should logic be reserved for the Executed command

  if(this.SelectedObject.Status == 1)
     //Do stuff
  else
     //Don't do stuff
}

It seems redundant to have a can execute method if we do additional data validation within the executed method.

DotNetRussell
  • 9,716
  • 10
  • 56
  • 111
  • 1
    Move the data validation stuff in the other methods into `CanExecute()`? – DGibbs Sep 25 '14 at 12:00
  • `I know this seems primarily opinion based` and `but I think that there is a specific answer to this` two totally different things :) – Shaharyar Sep 25 '14 at 12:00
  • @Shaharyar I am looking for someone like John Skeet to weigh in and say if there is an industry standard. – DotNetRussell Sep 25 '14 at 12:01
  • 2
    @AnthonyRussell then don't post a question, send him a message... – Adriano Repetti Sep 25 '14 at 12:04
  • In general, I use `CanExecute()` to check those conditions which *must* hold true in order for the command to execute. The first line of my `Execute()` method is typically something like `if (!CanExecute()) /* abort */;`. This guards against subtle bugs where `CanExecute()` is not reevaluated as often as it should be. – Mike Strobel Sep 25 '14 at 15:02
  • @MikeStrobel why would you call can execute within your execute? I am referring to command binding specifically here. Where the XAML auto calls can execute for you using the ICommand interface. – DotNetRussell Sep 25 '14 at 17:13
  • @AnthonyRussell WPF only evaluates `CanExecute()` in response to the `CanExecuteChanged` event being fired. If the execution conditions change, but the event is not fired for some reason, then it will not be reevaluated. Thus, I typically check `CanExecute()` from within the `Execute()` handler just in case. – Mike Strobel Sep 25 '14 at 17:15

2 Answers2

2

The way that I see it is that there is a distinction of whether something CAN happen and if something SHOULD happen.

An example of this can be a save button or something. The user may not have rights to save an entity so the action CAN'T happen.

If the user does have rights, all of the required fields may not be filled in so it SHOULDN'T happen.

Its in the semantics.

David Pilkington
  • 13,528
  • 3
  • 41
  • 73
  • Great answer! I'd like to also point out that having the CanExecute() also allows for controls' states to be set by the resulting boolean -- it's a nice-to-have sort of thing. – Andrew Mack Sep 25 '14 at 12:07
2

if the logic of your command assumes, that it must not be executed, when some conditions have met, then CanExecute have to check these conditions.

Otherwise, CanExecute must return true.

It doesn't matter, what is the nature of conditions, but you should note, that long running checks may hit UI thread performance.

Dennis
  • 37,026
  • 10
  • 82
  • 150
  • 1
    This was my concern as well. Since CanExecute seems to constantly fire I would be worried about any conditions that carry heavy overhead – DotNetRussell Sep 25 '14 at 12:09
  • @AnthonyRussell: so, what confuses you? – Dennis Sep 25 '14 at 12:13
  • I just wanted to know if there was an industry standard as far as if only null checks or other primitive checks should go in the CanExecute or if it was acceptable to put complex conditional logic in there – DotNetRussell Sep 25 '14 at 12:15
  • 2
    @AnthonyRussell there is not _industry standard_ but _common sense_. If, for example, to perform that check you need to scan a slow network connection and it takes 5 seconds then...do not do it in CanExecute. In general you should check everything you need to answer such question (_"can I execute this command now?"_). If one of that checks is slow (in a measured way) and/or resource consuming (for example if you have to lock a database table to perform a query) then move it to Execute() and add a comment in your code to explain the reason (in case it's not obvious). – Adriano Repetti Sep 25 '14 at 13:08