Limit your abstractions: Refactoring toward reduced abstractions

So in my previous post I spoke about this code and the complexity behind it:

public class CargoAdminController : BaseController
{
  [AcceptVerbs(HttpVerbs.Post)]
  public ActionResult Register(
      [ModelBinder(typeof (RegistrationCommandBinder))] RegistrationCommand registrationCommand)
  {
      DateTime arrivalDeadlineDateTime = DateTime.ParseExact(registrationCommand.ArrivalDeadline, 
                                                 RegisterDateFormat,CultureInfo.InvariantCulture);
      string trackingId = BookingServiceFacade.BookNewCargo(registrationCommand.OriginUnlocode, 
                                registrationCommand.DestinationUnlocode, arrivalDeadlineDateTime);
      return RedirectToAction(ShowActionName, new RouteValueDictionary(new {trackingId}));
  }
}

In this post, I intend to show how we can refactor things. I am going to do that by flattening the architecture, removing useless abstractions and creating a simpler, easier to work with system.

The first thing to do is to refactor the method signature:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Register(string originUnlocode, string destinationUnlocode, DateTime arrivalDeadline)

Those are three parameters that we need, there is no need to create a model binder, custom command, etc just for this. For that matter, if you already have a model binder, why on earth do you store the date as a string, and not a date time. The framework is quite happy to do the conversion for me, and if it can’t, I can extend the infrastructure to do so. I don’t need to patch this action with date parsing code.

Next, we have this notion of booking a new cargo, looking at the service, that looks like:

public string BookNewCargo(string origin, string destination, DateTime arrivalDeadline)
{
    try
    {
        TrackingId trackingId = BookingService.BookNewCargo(
            new UnLocode(origin),
            new UnLocode(destination),
            arrivalDeadline
            );
        return trackingId.IdString;
    }
    catch (Exception exception)
    {
        throw new NDDDRemoteBookingException(exception.Message);
    }
}

The error handling alone sets my teeth on edge. Also, note that we have a complex type for TrackingId, which contains just a string (there is a lot of code there for IValueObject<T>, comparison, etc), all of which basically go away if you use an actual string. The same is true for UnLocode (UN Location Code, I assume), but at least this one has some validation code in it.

Then there is the lovely forwarding call, which translate to:

public TrackingId BookNewCargo(UnLocode originUnLocode,
                               UnLocode destinationUnLocode,
                               DateTime arrivalDeadline)
{
    using (var transactionScope = new TransactionScope())
    {
        TrackingId trackingId = cargoRepository.NextTrackingId();
        Location origin = locationRepository.Find(originUnLocode);
        Location destination = locationRepository.Find(destinationUnLocode);
        Cargo cargo = CargoFactory.NewCargo(trackingId, origin, destination, arrivalDeadline);
        cargoRepository.Store(cargo);
        logger.Info("Booked new cargo with tracking id " + cargo.TrackingId);
        transactionScope.Complete();
        return cargo.TrackingId;
    }
}

And now we got somewhere, we actually have something there that is actually meaningful. I’ll skip going deeper, I am pretty sure that you can understand what is going on.

From my point of view of the common abstractions in an application:

1. Controllers
2. Views
3. Entities
4. Commands
5. Tasks
6. Events
7. Queries

Controllers are at the boundaries of the system, they orchestrate the entire system behavior. Note that I have no place for services or repositories in this list. That is quite intentional. Instead of going that route.

Take a look at the code that I ended up with: >

[AcceptVerbs(HttpVerbs.Post)]
 public ActionResult Register(string originUnlocode, string destinationUnlocode, DateTime arrivalDeadline)
 {
     var origin = Session.Load<Location>(originUnlocode);
     var destination = Session.Load<Location>(destinationUnlocode);
     var trackingId = Query(new NextTrackingIdQuery());
     var routeSpecification = new RouteSpecification(origin, destination, arrivalDeadline);
     var cargo = new Cargo(trackingId, routeSpecification);
     Session.Store(cargo);
     return RedirectToAction(ShowActionName, new RouteValueDictionary(new {trackingId}));
 }

As you can see, the entire architecture was collapsed into a single method.

And what kind of abstractions do we have here?

Well, we have the usual things from MVC, Controller, Action, parameter binding.

We have the session that we are using to load data by id, and to store the newly create cargo.

And we have the notion of a query. Generating a new TrackingID is a query that happen on the database (actually implemented as a hilo sequence). That is something that is definitely not the responsibility of the controller action, so we moved it into a query. Note that we have the Query() method there. It is defined as:

protected TResult Query<TResult>(Query<TResult> query)

And NextTrackingIdQuery is defined as:

public class NextTrackingIdQuery : Query<string>

Pretty simple, overall. And I can hear the nitpickers climb over the fences, waving the pitchforks and torches. “What happen when you need to reuse this logic? It is not in the UI and …”

There are a couple of things to note here.

First, there isn’t anywhere else that needs to book a cargo. And saying “and what happen when…” flies right into a wall of people shouting YAGNI.

Second, let us assume that there is such a need, to reuse the booking cargo scenario. How would we approach this?

Well, we can encapsulate the logic for the controller inside a Command. Which gives us:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Register(string originUnlocode, string destinationUnlocode, DateTime arrivalDeadline)
{
    var trackingId = ExecuteCommand(new RegisterCargo
    {
        OriginCode = originUnlocode,
        DestinationCode = destinationUnlocode,
        ArrivalDeadline = arrivalDeadline
    });
    return RedirectToAction(ShowActionName, new RouteValueDictionary(new { trackingId }));
}

And then we have the actual RegisterCargo command:

public abstract class Command
{
    public IDocumentSession Session { get; set; }
    public abstract void Execute();
    protected TResult Query<TResult>(Query<TResult> query);
}

public abstract class Command<T> : Command
{
    public T Result { get; protected set; }
}
public class RegisterCargo : Command<string>
{
    public override void Execute()
    {
        var origin = Session.Load<Location>(OriginCode);
        var destination = Session.Load<Location>(DestinationCode);
        var trackingId = Query(new NextTrackingIdQuery());
        var routeSpecification = new RouteSpecification(origin, destination, ArrivalDeadline);
        var cargo = new Cargo(trackingId, routeSpecification);
        Session.Save(cargo);
        Result = trackingId;
    }
    public string OriginCode { get; set; }
    public string DestinationCode { get; set; }
    public DateTime ArrivalDeadline { get; set; }
}

Note that the Command class also have a way to execute queries, in fact, it is the exact same way that we use when we had the code in the controller. We just moved stuff around, not really made any major change, but we can easily start using the same functionality in another location.

I generally don’t like doing this because most functionality is not reused, it is specific for a particular place and scenario, but I wanted to show how you can lift some part of the code and move it to a different location, otherwise people would complain about the “lack of reuse opportunities”.

On my next post I am going to talk about the Query() and ExecuteCommand() methods, and why they are so important.

Reference: Limit your abstractions: Refactoring toward reduced abstractions from our NCG partner Oren Eini at the Ayende @ Rahien blog.