Apr 3, 2008 at 11:11 PM
Edited Apr 3, 2008 at 11:38 PM
First, should it be renamed CompositeCommand?

Second, I'd love so see more LINQ action :)

Instead of:
public bool CanExecute(object parameter)
  bool hasEnabledCommandsThatShouldBeExecuted = false;
  foreach (var command in registeredCommands)
    if (ShouldExecute(command))
      if (!command.CanExecute(parameter))
        return false;
      hasEnabledCommandsThatShouldBeExecuted = true;
  return hasEnabledCommandsThatShouldBeExecuted;

LINQ allows:
public bool CanExecute(object parameter)
  return registeredCommands
    .TakeWhile(command => command.CanExecute(parameter))

Do you really want to know which one I prefer ? ;)
Apr 4, 2008 at 3:56 PM
They're actually not semantically the same:

The verbose version stops checking at the first command that can't be executed and immediately returns false.
The Linq version returns true if there are any that can and should execute

The Linq version seems more logically correct; it seems like the verbose version has a bug:
- If there are any commands that can't execute, then return false to CanExecute even though there may be 10 other commands that can execute.
Apr 4, 2008 at 4:32 PM
Let's split the Linq version into pieces:

  • It will iterate all those commands that Should Execute and STOP as soon as it meets one that CAN'T execute. AFAIK, that's the semantic of TakeWhile
  • Any will return whether there is any element yielded. That is determined by the TakeWhile which might yield nothing if the first command cannot be executed.

I don't see the bug on the explicit version and I don't see how they are different.

Can you elaborate?

Apr 4, 2008 at 4:39 PM
Ah I see your point. You're saying that CanExecute should return true if any command SHOULD execute and CAN execute.

Depends what really was the intent of this method but my the bool variable name, looks like you're right.

So the LINQ version should probably be:

public bool CanExecute(object parameter)
  return registeredCommands
    .Any(command => command.CanExecute(parameter));
Apr 4, 2008 at 4:43 PM
Edited Apr 4, 2008 at 4:48 PM
Being new to Linq I'm curious as to whether or not it is semantically the same?

Doesn't the TakeWhile operator only yield while it is true? Then the Any() would return the boolean value of the operation resulting in a false being returned the first time a false is encountered?

Edited: on the assumption that the first iteration returned false.

Apr 4, 2008 at 5:09 PM
BTW, enjoying your LINQ messages; really raises an eyebrow on the power of LINQ. I didn't catch your reponse until after I wrote mine but was curious...

Understanding that by naming convention RTaylor is correct, how would you make them semantically match? e.g., return false if any of the CanExecutes return false?

Apr 4, 2008 at 6:22 PM
Edited Apr 4, 2008 at 6:25 PM
To be more concrete, say there are 2 commands in registeredCommands; the first one will return true to CanExecute, and the second will return false - and that they both "ShouldExecute".

In the verbose/explicit/imperative version:
  • hasEnabledCommandsThatShouldBeExecuted is false by default
  • on the first iteration, hasEnabledCommandsThatShouldBeExecuted is set to true
  • on the second iteration, it falls into the inner-most if-statement and immediately returns false; ignoring the fact that we, in fact, just found one that CanExecute on the previous iteration
  • this method will return false if any registeredCommands cannot execute

In the original succinct/Linqified/declarative version :
  • the result of Any() will be true as long as there is at least one command that CanExecute before any commands that cannot
  • if the commands are reversed so that the first one returns false, then TakeWhile is short-circuited on the first iteration and Any() will return false
  • the behaviour of this version depends on the order of the commands

In the modified Linq version:
  • the result of Any() will be true as long as any of the commands CanExecute

Without understanding the actual requirements of the method, the original imperative version seems very odd to me and the modified Linq version seems the most logical.
Apr 4, 2008 at 7:30 PM
Edited Apr 4, 2008 at 7:37 PM

RTaylor wrote:
Without understanding the actual requirements of the method, the original imperative version seems very odd to me and the modified Linq version seems the most logical.

Your instincts seemed to be correct - I changed only one value in the Unit test (IsActive = false) below and the test fail. I plugged in francois_tanguay's LINQ code and the Unit Test passed.

        public void ActiveAwareDispatchCommandShouldIgnoreInActiveCommandsInCanExecuteVote()
            ActiveAwareMultiDispatchCommand activeAwareCommand = new ActiveAwareMultiDispatchCommand();
            MockActiveAwareCommand commandOne = new MockActiveAwareCommand() {IsActive = true, IsValid = false};
            MockActiveAwareCommand commandTwo = new MockActiveAwareCommand() { IsActive = true, IsValid = true};

Edited: I just ran the entire test suite and two other test fail so I guess "without understanding the actual requirements of the method"......
Apr 12, 2008 at 3:19 AM
Thanks Francois

We actually had several folks suggest it should be CompositeCommand, and we are renaming it. As to using LINQ, I will forward this on to the team.