CommandProcessor ignores handler with a TResult

[seif]

Just looking at the code in

Sharp-Architecture/Solutions/SharpArch.Domain/Commands/CommandProcessor.cs

Lines 13 to 27 in 1197484

	public void Process<TCommand>(TCommand command) where TCommand : ICommand
	{
	Validator.ValidateObject(command, new ValidationContext(command, null, null), true);
	var handlers = ServiceLocator.Current.GetAllInstances<ICommandHandler<TCommand>>();
	if (handlers == null || !handlers.Any())
	{
	throw new CommandHandlerNotFoundException(typeof(TCommand));
	}
	foreach (var handler in handlers)
	{
	handler.Handle(command);
	}
	}

and

Sharp-Architecture/Solutions/SharpArch.Domain/Commands/CommandProcessor.cs

Line 33 in 1197484

var handlers = ServiceLocator.Current.GetAllInstances<ICommandHandler<TCommand, TResult>>();

I think there would be a problem where if the caller didn't care about the result, and just wanted to fire the command and there was a handler registered for the command but implementing ICommandHandler<in TCommand, out TResult>, then that handler would not be called.

Do we want it to be called? What should we do for the other way around? where the user cared for the result and called the overload with the TResult, handlers that implement the ICommandHandler would not be triggered, should they be called but without adding any result to the return Enumeration?

[sandord]

I had a discussion about multiple command handlers with a colleague a few years ago. He stated that it doesn't make sense to have multiple command handlers handle the same command. One of his arguments were that you have no idea in which order the command handlers will be executed. I tend to agree with this.

Can you give a real world example where implementing multiple command handlers (which will be executed in no particular order) makes sense?

If not, we may consider simplifying the whole thing by supporting only one command handler per command. Knowing that there is only one registered command handler (which should be asserted), we can now have the command processor discard the result if the user didn't ask for one and throw an exception when he asks for a result while there isn't.

[seif]

I can't think of a real world example, but the code as it currently stands allows it, which is not right. +1 for having only 1 registered command handler and following @sandord in the last paragraph. If we want to do multiple things when a command is fired, then we should publish an event, and have event handlers to do the other work.

[sandord]

Perhaps we should get rid of the overload that takes a result handler. To my taste, it is superfluous since only one result instance will be available and it can simply be returned as a method result.

[seif]

I see your point about the overload with the delegate, I guess it is a matter of preference and my preference is not to have a result at all :) but there was quite a few people who wanted results from command handler which is why it was added.

Let's give someone a chance to object before doing it, give it a week or so.

[sandord]

Yes but it was added mainly because multiple results could be returned due to the former possibility to have multiple command handlers for a single command. Now that we have only one result at most, there is no need to have the overload since the method can simply return the return value. So the real reason I'd like to see it go is because it is redundant. But sure, I'll hold my breath for a while :)

[seif]

And add docs for command processing/dispatching and event handling.

[cd21h]

I'd rather replace command processor with https://github.com/jbogard/MediatR.
For current version we can mark it as obsolete.
@seif, What do you think?

[seif]

@Shatl sorry for late reply, was on holiday and then needed to look at the code for MediatR and see what it does.

I was worried about that the async stuff is actually spinning up tasks, but it leaves that to the implementors. The only issue I see is the lack of a Send that doesn't have a reply. i.e. MediatR only handles the event publishing and request/response style not a trigger of a command or a Send without a response.

Ignore the above, did some more digging in the code, and saw https://github.com/jbogard/MediatR/blob/master/src/MediatR/Unit.cs and the abstract class of RequestHandler we'd need to use that in an example for TResponse showing how to use the mediator with it.

I don't see a reason why not to depend on it, it doesn't have any dependencies that would conflict with ours. :)

[cd21h]

@seif, I believe we can close the issue then. For users who want to use Command/Query pattern, let's recommend https://github.com/jbogard/MediatR .

[seif]

Sure, wanna raise a ticket to delete the command processing code?