Ayende @ Rahien

It's a girl

Limit your abstractions: And how do you handle testing?

On my previous post, I explained about the value of pushing as much as possible to the infrastructure, and then show some code that showed how to do so. First, let us look at the business level code:

[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 }));
}

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; }
}

And the infrastructure code, now:

protected void Default_ExecuteCommand(Command cmd)
{
    cmd.Session = Session;
    cmd.Execute();
}

protected TResult Default_ExecuteCommand<TResult>(Command<TResult> cmd)
{
    ExecuteCommand((Command) cmd);
    return cmd.Result;
}

You might have noticed a problem in the way we are named things, the names on the action and the infrastructure code do not match. What is going on?

Well, the answer is quite simple. Let us look at how our controller looks like ( at least, the important parts ):

public class AbstractController : Controller
{
    public ISession Session;

    public Action<Command> AlternativeExecuteCommand { get; set; }
    public Func<Command, object> AlternativeExecuteCommandWithResult { get; set; }

    public void ExecuteCommand(Command cmd)
    {
        if (AlternativeExecuteCommand!= null)
            AlternativeExecuteCommand(cmd);
        else
            Default_ExecuteCommand(cmd);
    }

    public TResult ExecuteCommand<TResult>(Command<TResult> cmd)
    {
        if (AlternativeExecuteCommandWithResult != null)
            return (TResult)AlternativeExecuteCommandWithResult(cmd);
        return Default_ExecuteCommand(cmd);
    }

    protected void Default_ExecuteCommand(Command cmd)
    {
        cmd.Session = Session;
        cmd.Execute();
    }

    protected TResult Default_ExecuteCommand<TResult>(Command<TResult> cmd)
    {
        ExecuteCommand((Command)cmd);
        return cmd.Result;
    }
}

What?! You do mocking by hand and inject them like that? That is horrible! It is much easier to use a mocking framework and ….

Yes, it would be, if I was trying to mocking different things all the time. But given that I have very few abstractions, it make sense to not only build this sort of infrastructure, but to also build infrastructure for those things _in the tests_.

For example, let us write the test for the action:

[Fact]
public void WillRegisterCargo()
{
  ExecuteAction<CargoAdminController>( c=> c.Register("US", "UK", DateTime.Today) );
  
  Assert.IsType<RegisterCargo>( this.ExecutedCommands[0] );
}

Lego-mob 2The ExecuteAction method belongs to the test infrastructure, and it setups the controller to be run under the test scenario. Which allows me to not execute the command, but to actually get it.

From there, it is very easy to get to things like:

[Fact]
public void WillCreateNewCargoWithNewTrackingId()
{
  SetupQueryResponse<NextTrackingIdQuery>("abc");
  ExecuteCommand<RegisterCargo>( new RegisterCargo
  {
    OriginCode = "US",
    DestinationCode= "UK",
    ArrivalDeadline = DateTime.Today
  });
  
  var cargo = Session.Load<Cargo>("cargos/1");
  Assert.Equal("abc", cargo.TrackingId);
}

This is important, because now what you are testing is the actual interaction. You don’t care about any of the actual dependencies, we just abstracted them out, but without creating ton of interfaces, abstractions on top of abstractions or any of that.

In fact, we kept the number of abstractions to a minimum, and we can change pretty much every part of the system with very little fear of cascading change.

We have similar lego pieces, all of them move together and interact with one another with complete freedom, and we don’t have to have a Abstract Factory Factory Façade Factory.

Comments

Juanma
02/22/2012 08:26 AM by
Juanma

I like this solutions, specially not using mocks and testing the actual interaction.

How do yo deal with external dependencies, like a printing a document o sending an email? Do you abstract them behind and interface and inject that interface into the Task or let the task handle them directly?

Ryan Heath
02/22/2012 10:57 AM by
Ryan Heath

You could make void ExecuteCommand(Command cmd) TResult ExecuteCommand(Command cmd)

virtuals (with default impl) in your AbstractController.

Then override those in a TestAbstractController so you can still not execute them when calling the testmethod ExecuteAction.

By then you have the testing code/infrastucture where it belongs and it also makes the 'real' code much cleaner to read and easier to understand.

// Ryan

SteveR
02/22/2012 11:15 AM by
SteveR

I know that it's adding another abstraction, and the aim here is reduce not increase them, but I think I would go the extra step of introducing a CommandExecutor and injecting an implementation of that into the AbstractController instead. In production the default CommandExecutor could be injected by IOC and for your tests you could use an alternative TestCommandExecutor that would intercept the Commands executed so that your test infrastructure could record the Commands that were sent.

Jack
02/22/2012 11:16 AM by
Jack

Thanks for this thought provoking series.

The database session handling for commands seems to work only in trivial cases. How do you handle cases where a command issues a Raven store/update with OptimisticConcurrency = true? Where do you call SaveChanges and catch the ConcurrencyException and retry or bail out? You can't do it in the infrastructure because it requires command specific logic.

Generally speaking, do you call SaveChanges only from the place that created the session, or do you call SaveChanges also from classes that are injected with the session? To me it seems more robust that each command opens up a new db session, calls SaveChanges and handles the exceptions, instead of having a single db session for the whole http request and calling SaveChanges here and there, but unfortunately it's less efficient. What do you think?

If a Raven session throws ConcurrencyException, is the session still valid and usable after that? I remember reading that Raven sessions are invalid after an exception is thrown.

Ayende Rahien
02/22/2012 11:27 AM by
Ayende Rahien

Jack, The command executer handles things like deciding when to create a new session and when to call SaveChanges(). If you need to handle concurrency, you need to decide what the behavior is for the entire operation, not just the command. Is it okay to continue? Is it okay to retry? That is all something that you may put in either your command executer (if it is common) or in that specific scenario, if it is just there.

Jason Meckley
02/22/2012 11:35 AM by
Jason Meckley

@Jack. I have used action filters to manage the scope of session with great success. Before execute open session. After execute if ex = null save else rollback. Dispose session.

Conceptually an action with issue either a read or a write. Never both. Using post/redirect/get allows for the common save changes and reload the page.

Falhar
02/22/2012 04:19 PM by
Falhar

Dependencies as public properties still makes my spine shiver. The command class doesn't clearly comunicate, that it needs those dependencies for it to function. So, like it was said, someone else, who is using that command might forget to set this dependency.

I understand Session is so importatn, that every command has it. But like someone in comments said, any additional dependency will make the command code less clear.

This whole series makes me wonder, in what teams of people did you work and especialy how many people there were. Because unless the team knew this kind of architecture perfectly and knew what exactly each command does, then it would be hell to look for dependency errors.

Shane Courtrille
02/22/2012 04:25 PM by
Shane Courtrille

@Falhar: If your team doesn't understand an architecture this simple then you've got MUCH bigger problems on your hand :)

Matthew Shapiro
02/22/2012 04:35 PM by
Matthew Shapiro

You can pretty trivially solve the dependencies as properties issues by just creating a [Required] attribute, and have your command base class make sure that all properties with a [Required] attribute are not null prior to actually running the command's overridden execute() method. Then any developer will get an error explicitely showing a dependency was not set.

Falhar
02/22/2012 04:48 PM by
Falhar

@Shane: It is not about architecture. It is about specific commands, that need specific dependencies. Also, Session is specific dependency, because it's lifetime is quite specific.

@Matthew: Why should I add more infrastructure, if constructor and simple factory can handle such a thing? And in cleaner OOP way. I like attributes, but lets not abuse them here.

Falhar
02/22/2012 04:51 PM by
Falhar

I should really read my comments twice. Specific three times in one sentence sound quite bad.

Shane Courtrille
02/22/2012 05:02 PM by
Shane Courtrille

@Falhar: The person using that command shouldn't be setting the Session dependency. That's the job of the ExecuteCommand method which exists on the AbstractController. And when you're testing it looks like it's being handled by the testing architecture as well.

Falhar
02/22/2012 05:10 PM by
Falhar

@Shane: Thats why I said Session is specific one. What about other dependencies this command might have?

Shane Courtrille
02/22/2012 05:56 PM by
Shane Courtrille

@Falhar: I assume you would run it through a DI containers BuildUp method.. but maybe Oren has another trick up his sleeve?

I have done the alternative where I had a factory that was responsible for creating commands and making sure they had everything they needed and I didn't like it in the end. Much easier to just new something up and have property based injection.

Wayne M
02/22/2012 07:01 PM by
Wayne M

The one thing that keeps me unsure of this whole "Query" pattern thing is where you determine if you need a separate Query class to encapsulate the querying logic or not. For instance looking at the application we have at work (all DataSets and Stored Procs, but just curious how it would work with real patterns) some common cases are things like querying a single item by a single field, by multiple fields, updating some but not all properties, verify login/password, check if single DB property has been set, etc.

Would each of these be a query, resulting in having a dozen different query classes with names like LoginCustomerQuery (should probably be a Command, anyways), CustomerAgreedToTermsQuery and the like? Should the query classes only exist for actual complex queries and use the raw Session with AddCriteria and friends for anything not complex?

Matthew Shapiro
02/22/2012 07:56 PM by
Matthew Shapiro

@Falhar Unless I am mis-understanding what you are saying I think you should do this in infrastructure because (imho) using constructors to inject dependencies into commands (written the same way as Ayende is showing at least) makes it harder to read or write and requires more remembering by your staff. If the dependency is coming from DI, then your infrastructure should handle those dependencies and they should be invisible to your end developers when writing application code (as opposed to infrastructure code).

Nadav
02/22/2012 08:51 PM by
Nadav

Amazing link about the factory factory factory

Jeff Harris
02/23/2012 01:30 AM by
Jeff Harris

This series of posts has been amazing, and great comments from everyone. I'm seriously re-thinking the way I've been thinking about developing "good" applications.

It's interesting to see a lot of the objections to this approach center around the main question of "what if the problem was more complicated than this?"

I think we should add trying to solve arbitrarily complex problems with a single solution to the list of things that are "the root of all evil". Evidence: EF's EntityContext, ASP.Net's WebForms, some of my own code, etc.

Add enough abstractions and people stop understanding what's actually happening. Evidence: Mortgage-backed securities and just try googling "entitycontext"...1st two hits are about Java, 4th hit is a question of where to "store" the context between page requests in ASP.net...

João P. Bragança
02/23/2012 02:50 AM by
João P. Bragança

@Falhar,

Once you get past the idea that you need a ton of façades everywhere, you'll find that generally speaking, you end up with very few dependencies. As in one or two. The persistence mechanism, and if/when it gets complicated enough, message dispatching. Then you can do whatever kind of injection you like on your message handling class.

Remember, every required dependency that a class has means it has one more reason to change.

Lucas Ontivero
02/23/2012 02:56 AM by
Lucas Ontivero

I was waiting for this entry for a long time and I like the approach because I think most of the abstractions are created just for UT and, even when that is wrong, it is not easy to find good examples about how to do it without them. Thanks Ayende.

Bill
02/23/2012 05:06 PM by
Bill

All well and good but I find unit testing controller glue code is mostly a waste of time imho.

There are certain types of code that are prime candidates for unit testing. Testing that you set a property on some object properly is pointless.

Unfortunately a lot of people seem to blindly follow the tdd mantra without actually understanding what they are trying to get out of it.

Sony Mathew
02/23/2012 10:43 PM by
Sony Mathew

Your commands are all constructed explicitly and therefore tightly coupled to each other - you could not rewire with different implementation when necessary. You could create interfaces to all your commands but then you've over-abstracted.

Also Very likely that the persistence operations for Cargo will be shared under different use-cases (e.g. InspectCargo might require similar persistence) - so you'd likely end up with SaveCargo and other CRUD commands for Cargo. You are going to end up with a LOT of commands over the course of a project.

The reason for having stereotypes like Control, Service, Store (or DAO) etc is for the ability to evolve over a period of time. You discount the importance of this. If you don't have a designated place to put business logic, people will put them willy-nilly anywhere as a product evolves - usually right at the 1st layer UI.

Established stereotypes help people digest and understand code better contrary to what you may believe, people come to know what each stereotype will hold, its purpose and where to modify things.

Ryan
02/24/2012 03:14 AM by
Ryan

@Sony "Service" and "DAO" are extremely loaded terms. Every developer/team as a different idea of what they mean. "Query" and "Command" are much less ambiguous. The architecture Ayende is promoting here forces developers to think about what they are doing and then is fairly rigid in how that is done. Compare "RegisterCargoCommand" to "CargoService".

Regarding a save command: Did you not notice that the sole line of persistence was Session.Store(cargo)? You can't really simply that.

Sony Mathew
02/24/2012 11:43 PM by
Sony Mathew

@Ryan "Command" is an even more ambiguous term and has been in use for a long long long time usually in a limited context like being bound to actions in a Desktop GUI app. Not saying Command style doesn't work - I know it does, I've tried this approach in the past but it will get hairy with lots of commands getting coupled unless you abstract them into interfaces. To avoid the coupling folks take a step further into events which are listened / processed by registered handlers thru a mediator.

Most coupling and somewhat complex to follow:
---------------------------------------------
QueryCargo : Query
RegisterCargo : Command
InspectCargo : Command
-- processed by --
Command.execute()

Least coupling: more complex to follow
--------------------------------------
RegisterCargo : Event
InspectCargo : Event
QueryCargo: Event
QueryCargoResults: ResponseEvent
-- processed by --
Processor.process(Event) : ResponseEvent
Listener.listen(Event)

Encapsulated Coupling: (State Driven Design - My Favorite)
-----------------------------------------------------------
RegisterCargo : State
InspectCargo : State
CargoFilter: Filter
-- processed by --
Service.read(id) : State
Service.read(Filter) : List
Service.update(State)

Encapsulated coupling, simplest, easiest to comprehend
-------------------------------------------------------
CargoService.register(Cargo)
CargoService.inspect(Cargo)
Sony Mathew
02/24/2012 11:47 PM by
Sony Mathew

Missing angle-brackets for Generics on Processor, Listener, Service and others on my previous comments.

e.g. Processor[Event], Listener[Event], Service[State], List[State] except with angle brackets :)

Ryan
02/25/2012 01:27 AM by
Ryan

@Sony Absolutely in a desktop app I would want to decouple UI events from commands, but in a web context you only get to issue one command per call context and you can't count on state being saved between calls. Anytime I need to introduce a state pattern (which I also like) I defer to an independent service and use some sort of messaging library like NServiceBus to pass on commands from the web frontend.

Arcan
02/27/2012 03:34 PM by
Arcan

Hi, first of all thank you for this i enjoyed reading your new posts everytime , i want to ask you about the events what are they ? and how are they implemented and did this have a relation with how you implemented the rule in your previous post http://ayende.com/blog/3545/enabling-change-by-hard-coding-everything-the-smart-way and how the OnSubmitOrder is implemented ? thank you in advance

Ayende Rahien
02/27/2012 03:39 PM by
Ayende Rahien

Arcan, Take a look at Domain Events, that would help explains that. I might blog about this in the future, but it is really a big topic that can't be easily answered in the space of a comment.

Arcan
02/27/2012 04:20 PM by
Arcan

Thank you for you fast answer i googled this and found good artciles by fowler udi and greg i will check that in details but is this the approach that you used in your previous post that i mentioned earlier ? thank you again, i will be waiting for your post

Arcan
02/27/2012 04:36 PM by
Arcan

It is good i think i understand how you do events i went back to effectus example and remebered the eventpublisher class even if in that application there is only one event but it is enough to understand :) but i still not understand the class OnOrderSubmit is implemented and how it works in the previous post i mentioned before, i hope i will get an answer and hopefully i don t disturb you too much

James
03/07/2012 02:53 AM by
James

It would be great if we could see the whole implementation of the base controller! Am attempting to implement this myself, but am stuck translating the Command class + ExecuteCommand/AlternativeExecuteCommand/Default_ExecuteCommand methods to the Query side of things.

I assume Query is pretty simple;

public abstract class Query { public abstract TResult Execute(); }

How does SetUpQueryResult work in a compiler-safe way? Help us Ayende, your our only hope! (aside; perhaps the result of Execute should be IEnumerable? or Query subclasses returning lists should be declared as, say, GetCustomersQuery<IEnumerable>?)

But then it seems not possible to specify an 'public Func<Query, ???> AlternativeQuery { get; set; }' method. How is the

Comments have been closed on this topic.