Open sjb-sjb opened 8 years ago
Closing: Too complex to maintain.
I've gone down this path with AsyncCommand
before. In my article on the subject, I go far enough to support cancellation built-in, and in my own projects I've gone further still with more "useful" options, including progress reports. The type followed a similar approach to your code.
The problem is, adding options to the AsyncCommand
class quickly causes maintainability problems. For this reason, with Mvvm.Async
, I've split out CancelCommand
from AsyncCommand
in an attempt to make the types smaller and more maintainable while retaining functionality.
A large part of this decision is my personal preference of writing small, composable classes rather than semantically complex classes controlled with flags. However, this is the third (or fourth?) iteration of AsyncCommand
, and I'm not pretending it's perfect yet.
To address each of the points individually:
Cancellation is enabled by combining AsyncCommand
with CancelCommand
. Earlier versions of AsyncCommand
did have CancelCommand
built-in, but I think it's cleaner split out.
Progress is best done using the standard IProgress
API.
Running on a background thread is better with an explicit call to Task.Run
in the application code, rather than as a flag.
CanExecute is definitely a horrible design, in particular because some genius decided to violate the standard C# event pattern. However, it is way too late to fix it now. Introducing a different paradign is an appropriate step for an MVVM framework, but not for a library like Mvvm.Async
which is intended for use with any MVVM app.
Preventing commands from concurrently executing is already supported by AsyncCommand
. By default, multiple executions of the same command are not allowed, but the user may override this by specifying a custom implementation of CanExecute
(and calling OnCanExecuteChanged
when the return value may change).
Hi Stephen,
Thanks for your comments here and in the other post, which I will respond to separately.
I think you do have to be more careful as a framework designer than I need to be as an application developer.
I don't want to argue against your approach. Having said that, since I've put the effort into the code already I may as well make a few comments.
On cancellation, when I first looked at the Mvvm.Async version, I had not read up on cancellation generally. I looked at it and didn't immediately see how to use it in connection with AsyncCommand. Of course now I realize that it is by handing the token in to the command delegate, which is what I do in the more complex version. The point is that simpler classes can sometimes be more complex to use. Because of this, I'm actually not fully convinced that simpler classes make the total application easier to maintain. Say we have two simple classes and maybe 3 lines of code are needed to use them together, vs one complex class that has the three lines built in, or maybe it takes 5 extra lines instead of 3 to get the single-class version. Say I use it 10 times in my application, then I've got 30 lines sprinkled around my application in the "simple" version vs 5 built into the "complex" class. Saying that I could always just make a third class containing the tie between the two simple classes doesn't really answer this conundrum, because that third class would basically be equal to what the more "complex" solution is: the two 'simple' classes just being subclasses defined within the 'complex' class. Just because the end class delivered to the user is complex doesn't mean that it has to be poorly designed internally -- it should define private subclasses when that is appropriate for good design. So the question is not how much overall maintenance there is, but where does the maintenance effort lie -- in the framework or in the application code.
What matters, in my view, isn't how simple or complex the individual pieces of code are, but whether they collectively deliver the functionality that is needed, and how much work the user has to do to access that functionality. The risk in delivering larger chunks of code as the end product isn't that the solution is more complex in aggregate -- as just discussed, it isn't more complex. The risk is that if the larger chunk of code doesn't hit the target then it will be harder for the user to reconfigure it into a different delivery package. If you're comfortable that you've delivered what is needed then the larger solution is better because it delivers more integrated functionality, eliminating the need for the user to tie the smaller classes together over and over again.
About preventing commands from executing simultaneously, I wanted to be able to have mutual exclusion between different commands, such as FileOpen, FileClose, FileSave. Of course I can roll it myself by defining all those CanExecutes and calling OnCanExecuteChanged everywhere. But that doesn't lead me at all toward a simpler application, especially if I have to do it repeatedly throughout my application -- see the comments above.
On Progress
All the best & happy coding Steve
I looked at it and didn't immediately see how to use it in connection with AsyncCommand
I am planning to write docs specifically for this, since it will definitely be a common use case.
I am also considering adding a creation method that will create an AsyncCommand
/CancelCommand
pair. I believe only a static method would be necessary, but a wrapper class may be easier. Note that there's a big difference in maintainability between merging CancelCommand
into AsyncCommand
(difficult to maintain) and keeping them separate and providing an AsyncCommand.CreateCancelableAsyncCommand()
method or a CancelableAsyncCommand
wrapper type (both easier to maintain).
On a side note, keeping classes smaller and more focused can increase usability. For example, if I wanted two or three AsyncCommand
s to be triggered independently, but have a single CancelCommand
to cancel them all.
I wanted to be able to have mutual exclusion between different commands
I think that's a very interesting use case! I'll play around with your AreMutuallyExclusive
approach - it'd be great to have a solution that does use CanExecute
and friends (since they're standard, not because they're good), and also be sufficiently generic.
Specifically, they don't really tie in to the property notification system.
Neither of those types are specifically for MVVM apps. I do have some other IProgress<T>
implementations that do support property notification, that I am planning to add to this library shortly.
Well, I agree with your comment about the value of having a CancellableAsyncCommand wrapper type. I suppose I could have, or perhaps should have, designed my version of AsyncCommand so that it had two subclasses rather than one.
On the mutually exclusive commands, here is how I actually use it (and also how I use the property notifications for controlling CanExecute):
public FileCommands()
{
this.ReopenCommand = new AsyncCommand( this.XX.FileReopenAsync);
this.FileOpenCommand = new AsyncCommand( this.XX.FileOpenAsync);
this.FileNewCommand = new AsyncCommand( this.XX.FileNewAsync);
this.FileCloseCommand = new AsyncCommand( this.XX.FileCloseAsync);
this.FileNewCommand.CanExecuteOnlyIf(
BoundFunction.Create(
this,
(canInitialize, fileIsOpen) => (canInitialize && !fileIsOpen),
(@this) => @this.XX.CanFileInitialize,
(@this) => @this.XX.FileIsOpen
)
);
this.FileOpenCommand.CanExecuteOnlyIf(
BoundFunction.Create(
this,
(fileIsOpen) => !fileIsOpen,
(@this) => @this.XX.FileIsOpen
)
);
this.FileCloseCommand.CanExecuteOnlyIf(
this, (@this) => @this.XX.FileIsOpen
);
AsyncCommand.AreMutuallyExclusive(
new AsyncCommand[] { ReopenCommand, FileOpenCommand, FileNewCommand, FileCloseCommand }
);
this.ExecutingCommand = BoundFunction.Create(
this,
(reopenIsExecuting, fileOpenIsExecuting, fileNewIsExecuting, fileCloseIsExecuting) => (IAsyncCommand)(reopenIsExecuting? this.ReopenCommand: (fileNewIsExecuting? this.FileNewCommand: (fileOpenIsExecuting? this.FileOpenCommand: (fileCloseIsExecuting? this.FileCloseCommand: null)))),
(@this) => @this.ReopenCommand.IsExecuting,
(@this) => @this.FileOpenCommand.IsExecuting,
(@this) => @this.FileNewCommand.IsExecuting,
(@this) => @this.FileCloseCommand.IsExecuting
);
this.ReopenCommand.Execute();
}
From: Stephen Cleary [mailto:notifications@github.com] Sent: August 20, 2016 8:14 AM To: StephenCleary/Mvvm.Async Cc: sjb; Author Subject: Re: [StephenCleary/Mvvm.Async] AsyncCommand (#3)
I looked at it and didn't immediately see how to use it in connection with AsyncCommand
I am planning to write docs specifically for this, since it will definitely be a common use case.
I am also considering adding a creation method that will create an AsyncCommand/CancelCommand pair. I believe only a static method would be necessary, but a wrapper class may be easier. Note that there's a big difference in maintainability between merging CancelCommand into AsyncCommand (difficult to maintain) and keeping them separate and providing an AsyncCommand.CreateCancelableAsyncCommand() method or a CancelableAsyncCommand wrapper type (both easier to maintain).
On a side note, keeping classes smaller and more focused can increase usability. For example, if I wanted two or three AsyncCommands to be triggered independently, but have a single CancelCommand to cancel them all.
I wanted to be able to have mutual exclusion between different commands
I think that's a very interesting use case! I'll play around with your AreMutuallyExclusive approach - it'd be great to have a solution that does use CanExecute and friends (since they're standard, not because they're good), and also be sufficiently generic.
Specifically, they don't really tie in to the property notification system.
Neither of those types are specifically for MVVM apps. I do have some other IProgress
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/StephenCleary/Mvvm.Async/issues/3#issuecomment-241196865 , or mute the thread https://github.com/notifications/unsubscribe-auth/ASQzRslnk0ZxEfXSdGoBMzvzyAGvS4v3ks5qhu91gaJpZM4JEyZi .Image removed by sender.
Hi Stephen,
Thought I would give you an update on my use case. What I found was that I wanted two distinct types of functionality.
One, I wanted a really robust, general-purpose delegate wrapper. The constructor input is either an async or sync delegate, which it can then execute either sync or async (well, not sync for an async delegate) and wrapping it in NotifyTask when appropriate. It can have a return value, it can execute on a background thread or the original thread. It creates a cancellation token if needed and a progress reporter object if needed. It moves the exceptions from the sync or async execution into the progress reporter. Also, via a derived class, I keep track of all the delegates and notify the application when interesting things happen.
Two, the command. It's a delegate wrapper that then deals with the various requirements of ICommand such as CanExecute, etc. I did stick with the approach of switching CanExecute to something based on INotifyPropertyChanged. Since the cancellation token has been moved into the delegate wrapper, it doesn't have the same level of complexity as discussed earlier. True some of this complexity has just been moved elsewhere -- there's no free lunch.
Thanks again for all the great code you have out there & for your dedication to the world of async!
sjb
P.S. One other comment. For NotifyTask<TResult>, I ran into cases where I needed it to be derived from NotifyTask. This is quite easily accomplished by using "new" on the Task property in the derived class. The reason I needed this was for a non-generic interface that exposed a NotifyTask property, where the implementation was generic and actually creating a NotifyTask<TResult>.
I have done a bit of work relating to AsyncCommand which I am hoping the community can benefit from.
I had the following objectives and approaches:
I am providing source code that accomplishes all this, and I am hereby entering it into the public domain for anyone to use without restriction. Please also refer to my other post discussing NotifyTask and ProgressReport.
sjb
AsyncCommand.txt BoundFunction.txt BoundAction.txt Property.txt