5
\$\begingroup\$
public class AddPostCommand
{
    public string Title { get; set; }
    public string Body { get; set; }
    public bool Published { get; set; }
}

public class AddPostCommandHandler
{
    public string Handle(AddPostCommand command)
    {
        var param = new
        {
            Slug = command.Title.Replace(" ", "-"),
            PublishDate = command.Published ? DateTime.Now : DateTime.MinValue,
            command.Title,
            command.Body,
            command.Published
        };

        // I removed the data access code for brevity.

        return param.Slug;
    }
}

I have a command and I have a command handler. I also have a controller that invokes the command handler.

Within the command handler I generate a slug (definition). In the database, the slug column has a unique constraint - there must be no duplicate slugs.

I need to find a clean way to inform the caller that they have effectively provided to the command handler a duplicate slug.

I do not perform any validation in the command handler yet. This is the simplest solution I can come up with:

PostRequestHandler requestHandler = new PostRequestHandler();
AddPostCommandHandler commandHandler = new AddPostCommandHandler();

[HttpPost]
public ActionResult AddPost(AddPostCommand command)
{
    string expectedSlug = command.Title.Replace(" ", "-");
    PostRequest request = new PostRequest { Slug = expectedSlug };
    PostModel post = requestHandler.Handle(request);
    if (post != null)
    {
        ModelState.AddModelError("", "A post with this title already exists.");
        return View(post);
    }

    string actualSlug = commandHandler.Handle(command);
    return RedirectToAction("Index", "Post", new { actualSlug });
}

I abhor this code.

My most pressing concern is that I do not want the caller to have to generate the slug. I am also concerned that the controller action is becoming too big.

How might I communicate to the controller that an error has occurred (so that an error message can be displayed to the user) in a clean way?

\$\endgroup\$
5
  • \$\begingroup\$ Interesting Question. Are you open to changes in architecture in a review, or just making it work with the command / command handler pattern? \$\endgroup\$
    – xDaevax
    Dec 9, 2014 at 18:07
  • \$\begingroup\$ @xDaevax I am highly curious about how to make this work but I am also open to discussing changes in architecture. Thank you for your comment. \$\endgroup\$ Dec 9, 2014 at 18:31
  • \$\begingroup\$ Maybe I didn't really get where the problem is, but what is the reason for not simply throwing an exception? \$\endgroup\$
    – conrad
    Dec 11, 2014 at 21:06
  • 1
    \$\begingroup\$ @conrad This isn't an exceptional scenario - using exceptions for control flow is a poor practice in my opinion. Thank you for your comment. \$\endgroup\$ Dec 11, 2014 at 21:30
  • \$\begingroup\$ Then the appropriate solution would be to provide a CanHandle method which checks the error beforehand and execute the Handle iff CanHandle returns true. Other solutions would be providing an error callback or even return an error code, but this is IMHO even worse than throwing an exception. \$\endgroup\$
    – conrad
    Dec 11, 2014 at 22:25

3 Answers 3

3
\$\begingroup\$

This is an interesting question because I've never seen the MVC and the command pattern implemented together. Typically I think WebForms when I think of the command pattern. I'm still on the fence on whether or not it is a good fit but there are a couple of MVC concepts you can use to make your code cleaner. The most valuable of which in your case is to:

Leverage Custom Model Binding

I agree, your controller shouldn't have to do work to build the request model (slug, post request), this should all be happening in a custom model binder:

public class AddPostCommandModelBinder : DefaultModelBinder
{

    protected virtual void OnModelUpdated(ControllerContext controllerContext, ModelBindingContext bindingContext)
    {
        var command = (AddPostCommand)bindingContext.Model;
        // Add the ExpectedSlug property to the request command and have the ModelBinder do this work (as well as any other post-binding logic
        command.ExpectedSlug = command.Title.Replace(" ", "-");
    }
}

In addition to this, your PostRequestHandler is a concern much better solved using Model Binding, as the responsibility of Model Binding is to take request parameters and map them to a strongly typed object as well as validating those inputs (by default using the DataAnnotations namespace: http://msdn.microsoft.com/en-us/library/system.componentmodel.dataannotations%28v=vs.110%29.aspx.

In yet another Model Binder, you can put any custom validation logic, for example:

public override Object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
{
    var newModel = (PostRequest)bindingContext.Model;
    newModel.AttemptedSlug = bindingContext.ValueProvider.GetValue("title").AttemptedValue;

    if(string.IsNullOrWhiteSpace(newModel.AttemptedSlug))
    {
        bindingContext.ModelState.AddModelError("AttemptedSlug", "The Attempted Slug is required.");
    }
    // Other custom logic

    return newModel;
}

A point on Injection

As with anything MVC, the code should be test-able. I'm not sure if you omitted it for brevity, but the command handlers should be constructor-injected interfaces bound to concrete types (ideally) using an IoC container. Also, with the changes above, your controller should now look like this:

public class PostController : Controller
{
    IPostCommandHandler _postCommandHandler;

    public PostController(IPostCommandHandler addCommandHandler)
    {
        this._postCommandHandler = addCommandHandler;
    }

    [HttpPost]
    public ActionResult AddPost(AddPostCommand command, PostRequest submitRequest)
    {
        ActionResult result = null;
        if (!submitRequest.IsValid)
        {
            result = View("AddPost", submitRequest);
        }
        else
        {
            string actualSlug = this._postCommandHandler.Handle(command);
            result = RedirectToAction("Index", "Post", new { actualSlug });
        }

        return result;
    }
}

Ultimately, I follow several rules of thumb as far as my separation of concerns that have kept me mostly out of trouble.

  • Model binders bind all request parameters to strongly typed request objects (with a few exceptions).
    • Model binders handle basic data-validation and error handling for user input
  • Controllers accept request models, check for validity, call external data services to build response view models, and determine action (redirect, view, etc...)

Other Notes

The usage of the word "post" as part of your domain (presumably you're using the term in the sense of a blog "post"), should be re-considered. POST carries a lot of context with it and some developers (myself included) will have to read around the common use of post when reading this code.

  • Consider renaming post to blogPost or forumPost or newsPost to make the code clearer.

Adding validation logic in the controller should be a red flag, but adding messaging in the controller is perfectly appropriate, as the messaging is directly related to the view (which is found, prepared and delivered by the controller). I wouldn't do a null check as you have done, but either use a validator in the model binder (ModelState.IsValid), or providing your own as I did above as part of the PostRequest object.

You may also want to consider adding an AddPostCommand property to your PostRequest, which would then require your controller to take only one argument. You can still have a custom model binder for each type.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your answer David. In my actual code I'm inverting my command handler dependencies except I'm using MediatR to route my messages. You make some good points and I thank you for them however I do have concerns: the caller still has to generate a slug which I think should be an implementation detail; I suspect that adding model binders in this case might be hard to follow - nonetheless, I will try it on for size. \$\endgroup\$ Dec 17, 2014 at 8:25
3
+50
\$\begingroup\$

From my experience, when you launch a command you should be 99.9% sure that it will work since the command pattern isn't really good to inform the "launcher" of the command status. So, I think that before your command gets into your command handler, it should be validated.

Then, if your command handler can't process the command, because of an operation that would happen between validation and handling from another thread for example, you should throw an exception.

public class AddPostCommand
{
    public string Title { get; set; }
    public string Body { get; set; }
    public bool Published { get; set; }
}

public class AddPostCommandHandler
{
    public string Handle(AddPostCommand command)
    {
        var param = new
        {
            Slug = command.Title.Replace(" ", "-"),
            PublishDate = command.Published ? DateTime.Now : DateTime.MinValue,
            command.Title,
            command.Body,
            command.Published
        };

        //DataAccess.DoesItExists?
        //Oh it does, snap.. throw new InvalidCommandException();
        //It doesn't, great. Let's keep on going

        return param.Slug;
    }
}

Your validation doesn't have to be in the controller, I'd even say it doesn't belong there. You should probably think about having Command validators that would be some other classes that say if your command can be launched.

I would want to give you more information, but the lack of context on the code makes it hard for me to show you how I would build the controller but it would go along these lines :

  1. Validate the command
  2. If invalid, return the view with the errors
  3. If valid, handle the command
  4. If command handler can't handle, throw exception (Which should be very rare considering you just validated it)
  5. If invalid, catch exception and return view with the errors
  6. It works, great!
\$\endgroup\$
2
\$\begingroup\$

You can create a base class which all your handlers will derive from. The base class will at least two properties as follows:

public class BaseHandler
{
    public bool HasErrors { get { return Errors.Any(); } }
    public List<string> Errors { get; set; }

    public BaseHandler()
    {
        Errors = new List<string>();
    }
}

Now you can derive your Handlers as follows:

public class AddPostCommandHandler : BaseHandler
{
    public string Handle(AddPostCommand command)
    {
        var param = new
        {
            Slug = command.Title.Replace(" ", "-"),
            PublishDate = command.Published ? DateTime.Now : DateTime.MinValue,
            command.Title,
            command.Body,
            command.Published
        };

        if(slugAlreadyExists)
            Errors.Add("slug already exists");

        return param.Slug;
    }
}

Controller:

[HttpPost]
public ActionResult AddPost(AddPostCommand command)
{
    string actualSlug = commandHandler.Handle(command);

    if (commandHandler.HasErrors)
    {
        //join the errors into a single HTML string
        ModelState.AddModelError("", string.Join("<br/>", commandHandler.Errors));

        return View();
    }


    return RedirectToAction("Index", "Post", new { actualSlug });
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.