I have Controller that basically publishes different Event based on some input criteria, simplified version of the logic is here:

[HttpPost]
public async Task<ActionResult> Create(CreateRequestDto request)
{

    IRegistrationEvent createAppEvent;
    switch (request.Kind)
    {
        case RegistrationKind.Client: 
            createAppEvent = this.mapper.Map<CreateClientModelCommand>(request);
            break;
        case RegistrationKind.Server:
            createAppEvent = this.mapper.Map<CreateServerModelCommand>(request);
            break;
        // case Other, ....
        default:
            throw new ArgumentException(nameof(request.Kind));
    }

    var created = await Mediator.Send(createAppEvent);
    this.cacheSet.Remove(CacheKey.ApplicationInfo);

    return CreatedAtAction(request.Kind);
}

Although I like moving the logic to the separate handlers, I’m wondering what is the best way to refactor this “switch” while it seems odd to me. I though about adopting strategy pattern, but ended up in similar way of resolving the strategy by some if-else/switch, just hidden from the Controller action.

2

As long as this is the one and only place in your code where the values of RegistrationKind are dispatched, there is probably nothing wrong with this method. It is just some variation of the factory method pattern.

The situation may only become “odd” when your code will contain this kind of switch in several places. If that’s the case, you may need another level of indirection:

  • one conventional factory which takes a RegistrationKind value, dispatches it by a switch statement and returns some kind of domain object representing a ClientStrategy, a ServerStrategy and what else you need, all with a commmon base class (or common interface) Strategy.

  • specific Strategy implementations in those classes which know about the specific events, the mapping and whatever is specific to your concept of a Client or a Server.

Sure, you could also try to implement some more data driven-approach and use a dispatch table (for example, a dictionary Dictionary<RegistrationKind,Strategy>) which will be initialized once at program start. But this is more adequate when you need to loop over all available strategy objects, or when you need to extend the available strategies dynamically at run time.

I would recommend to pick the most simple approach which implements your requirements, and only change to something different when start noticing repetitive code which needs to be changed in several places in case of extending requirements.

Controllers publishing events via a mediator like this is an antipattern in my eyes.

You already have the request -> controller action mapping, it’s part of the framework, just use routes.

Same problem with the mapping. You already have a json -> object mapping, you don’t need another one, or a dto.

I would be asking myself why haven’t I just got two controller methods

[HttpPost]
[Route("CreateClient")
public async Task<ActionResult> CreateClient(Client client)
{
   return await clientService.Create(client)
}

[HttpPost]
[Route("CreateServer")
public async Task<ActionResult> CreateServer(Server server)
{
   return await serverService.Create(server)
}

Or one action to rule them all

[HttpPost]
[Route("ExecuteAnyAndAllCommands")
public async Task<ActionResult> ExecuteAnyAndAllCommands(ICommand command) //still no need for mapper with the right json setup
{
   return await mediator.Send(command)
}

3

Thank you for all your suggestions. I went more on @Euphoric (at least I think so) direction and ended up with moving the logic of handling the request to the IApplicationEventResolver.cs with following declaration:

public interface IApplicationEventResolver
{
    IRegistrationEvent Resolve(CreateRequestDto request);
}

The implementation contains the beforementioned switch logic:

public class ApplicationEventResolver : IApplicationEventResolver
{
    private readonly IMapper mapper;

    public ApplicationEventResolver(IMapper mapper)
    {
        this.mapper = mapper;
    }

    /// <inheritdoc />
    public IRegistrationEvent Resolve(CreateRequestDto request)
    {
        IRegistrationEvent createAppEvent;
        switch (request.Kind)
        {
            case ApplicationKind.Client:
                createAppEvent = this.mapper.Map<CreateClientModelCommand>(request);
                break;
            case ApplicationKind.Server:
                createAppEvent = this.mapper.Map<CreateServerModelCommand>(request);
                break;
            default:
                throw new ArgumentException(nameof(request.Kind));
        }

        return createAppEvent;
    }
}

and finally the Controller looks like:

[HttpPost]
public async Task<ActionResult> Create(CreateRequestDto request)
{
    // eventResolver is instantiated by injecting it into the controller constructor, 
    // mapper is not needed anymore in the Controller.
    var createAppEvent = this.eventResolver.Resolve(request);
    var created = await Mediator.Send(createAppEvent);
    this.cacheSet.Remove(CacheKey.ApplicationInfo);

    return CreatedAtAction(request.Kind);

}

Please put your thoughts if you feel there is something bad or you have any improvements for this code.