Refactoring a C# Save Command Handler

I have the following command handler. The handler takes a command object and uses its properties to either create or update an entity.

It decides this by the Id property on the command object which is nullable. If null, then create, if not, then update.

public class SaveCategoryCommandHandler : ICommandHandler<SaveCategoryCommand>
{
    public SaveCategoryCommandHandler(
        ICategoryRepository<Category> categoryRepository,
        ITracker<User> tracker,
        IMapProcessor mapProcessor,
        IUnitOfWork unitOfWork,
        IPostCommitRegistrator registrator)
    {
         // Private fields are set up. The definitions for the fields have been removed for brevity.
    }

    public override void Handle(SaveCategoryCommand command)
    {
        // The only thing here that is important to the question is the below ternary operator.

            var category = command.Id.HasValue ? GetForUpdate(command) : Create(command);

 // Below code is not important to the question. It is common to both create and update operations though.

            MapProcessor.Map(command, category);

            UnitOfWork.Commit();

            Registrator.Committed += () =>
            {
                command.Id = category.Id;
            };

    }

    private Category GetForUpdate(SaveCategoryCommand command)
    {
        // Category is retrieved and tracking information added
    }

    private Category Create(SaveCategoryCommand command)
    {
        // Category is created via the ICategoryRepository and some other stuff happens too.
    }
}

I used to have two handlers, one for creating and one for updating, along with two commands for creating and updating. Everything was wired up using IoC.

After refactoring into one class to reduce the amount of code duplication I ended up with the above handler class. Another motivation for refactoring was to avoid having two commands (UpdateCategoryCommand and CreateCategoryCommand) which was leading to more duplication with validation and similar.

One example of this was having to have two validation decorators for what were effectively the same command (as they differed by only having an Id property). The decorators did implement inheritance but it is still a pain when there are a lot of commands to deal with.

There are a few things that bug me about the refactored handler.

One is the number of dependencies being injected. Another is that there is a lot going on the class. The if ternary bothers me - it seems like a bit of a code smell.

One option is to inject some sort of helper class into the handler. This could implement some sort of ICategoryHelper interface with concrete Create and Update implementations. This would mean the ICategoryRepository and ITracker dependencies could be replaced with a single dependency on ICategoryHelper.

The only potential issue is that this would require some sort of conditional injection from the IoC container based on whether the Id field on the Command was null or not.

I am using SimpleInjector and am unsure of the syntax of how to do this or even if it can be done at all.

Is this doing this via IoC also a smell, or should it be the handlers responsibility to do this?

Are there any other patterns or approaches for solving this problem? I had thought a decorator could possibly be used but I can't really think of how to approach doing it that way.

Solution

My experience is that having two separate commands (SaveCategoryCommand and UpdateCategoryCommand) with one command handler gives the best results (although two separate command handlers might sometimes be okay as well).

The commands should not inherit from a CategoryCommandBase base class, but instead the data that both commands share should be extracted to a DTO class that is exposed as a property on both classes (composition over inheritance). The command handler should implement both interfaces and this allows it to contain shared functionality.

[Permission(Permissions.CreateCategories)]
class SaveCategory {
    [Required, ValidateObject]
    public CategoryData Data;

    // Assuming name can't be changed after creation
    [Required, StringLength(50)]
    public string Name;
}

[Permission(Permissions.ManageCategories)]
class UpdateCategory {
    [NonEmptyGuid]
    public Guid CategoryId;

    [Required, ValidateObject]
    public CategoryData Data;
}

class CategoryData {
    [NonEmptyGuid]
    public Guid CategoryTypeId;
    [Required, StringLength(250)]
    public string Description;
}

Having two commands works best, because when every action has its own command, it makes it easier to log them, and allows them to give different permissions (using attributes for instance, as shown above). Having shared data object works best, because it allows you to pass it around in the command handler and allows the view to bind to it. And inheritance is almost always ugly.

class CategoryCommandHandler :
    ICommandHandler<SaveCategory>,
    ICommandHandler<UpdateCategory> {
    public CategoryCommandHandler() { }

    public void Handle(SaveCategory command) {
        var c = new Category { Name = command.Name };
        UpdateCategory(c, command.Data);
    }

    public void Handle(UpdateCategory command) {
        var c = this.repository.GetById(command.CategoryId);
        UpdateCategory(c, command.Data);
    }

    private void UpdateCategory(Category cat, CategoryData data) {
        cat.CategoryTypeId = data.CategoryDataId;
        cat.Description = data.Description;
    }
}

Do note that CRUDy operations will always result in solutions that seem not as clean as that task based operations will. That's one of the many reasons I push developers and requiremwnt engineers to think about the tasks they want to perform. This results in better UI, greater UX, more expressive audit trails, more pleasant design, and better overall software. But some parts of your application will always be CRUDy; no matter what you do.