DeriveDeliveryProgress presumably updates the variables cargo.Delivery.XX? Yay non obvious side effects.

Surely the misdirected and arrival events should fire when they happen, and not at the next "inspection".

Some of the IApplicationEvents interface woudl probably be handled better through domain events. That would also introduce less coupling...

oh, and if we have a domain model then the events should probably be published from inside of it not from a service...

I don't understand why there's a differentiation between CargoWasMisdirected and CargoHasArrived. If they were replaced by CargoIsBeingInspected surely implementations of IApplicationEvents could inspect the cargo's delivery status?

But once you've replaced those two IApplicationEvent methods with a single method, and got rid of the if blocks, all InspectCargo does is load a cargo object, ask something else to inspect the cargo, and then store it back to the repository without even knowing that it's been changed.

It's basically an InspectCargo method that doesn't inspect any cargo.

As for the ICargoInspectionService interface, the implementation is so generic that why would you ever implement another one? The class is not on the system boundary, and because it's so generic, you're unlikely to be bothered about replacing it for unit testing.

It contains both code that retrieves the entities as well as code that does business logic (like deciding which events to fire).

You do have "Service locator" anti pattern everywhere instead receiving parameters in constructor ?

There is a lots of plumbing code, not actual business logic. It's carefuly mixed with DDD to looks great.

Actual bisuness logic looks like: GetHistory |> DeriveDeliveryProgress |> SendEvents where "|>" - pipeline operator for left-to-right fucntion composition. There is no need for Cargo instance, Repository, etc

Another large pain point - statuses in Boolean Fields and handlers in methods. Lack of polymorphism. Should be replaced with factory & event class hierarcy.

There is no need to store DeliveryProgress, it's can be calculated at any time, event if business logic changed.

@Frank , this class is like a controller : it doesn't do anything it just load the data and call the business logic. At some point you have to do it

I agree with Alex, the decoupling is useless here, mainly because this Service act like a controller, there is no behavior here, why would we need to abstract it ?

And I don't like the usage of event here : a composite pattern would have been enough, event are not made to handle business logic, but I think they were made for making react some other system/sub-system to an event occurring in a linked system/sub-system. It can be removed, the current system should work well, but here they handle the whole business logic.

Should InspectCargo not be implied by the CargoHasArrived method in the first place. Cargo has arrived is the main event and should invoke the inspection and alert certain people if it was a backlog package for instance.

It is not the job of the inspection service to call CargoHasArrived event. Inspection IMPLIES that something has arrived. If nothing did arrive, you have nothing to inspect in the first place.

Further methods like CargoHasArrived CargoWasMisredirected are the same thing. A package did arrived, whether it was misredirected is not important. But calling CargoHasArrived also implies that the cargo is handled. So, I must say that the IApplicationEvents interface is a bit confusing..

And I know that you are a strong advocate against repositories, so the repository pattern would not make you happy either..

I would have the Cargo class raise the events instead. Tell, dont ask!

I have no idea whats wrong there. Personally I do not like that much various services each doing a small part here and small part there. But I can not say that it is wrong way to do it.

One thing that really bugs me about code like this (OK maybe it's only a small point) is 'if (cargo == null)'. Here the null keyword is being overloaded with additional meaning from the English language definition of the word (which is very different from the C# definition of the word). It's often said that you shouldn't use exceptions to control program flow, personally I think using invalid pointers is even worse!

I'll bet my house you are going to say that the abstraction of database calls is going to cause performance problems and leave little option of perf-tuning when such problems arise.

If we remove the abstractions and look at the database calls being made:

• fetch the cargo

• fetch handling history of cargo

• set handling history on the cargo (DeriveDeliveryProgress) - ** This wont be a database call bit is relevant **

• update the cargo

So there are at least 3 database calls here - we don't know what is hidden inside the query methods - there could be multiple queries etc inside LookUpHandlingHistoryOfCargo.

My guess is also that the cargo doesn't even need to be involved here. The delivery details being attached to it are not needed. DDD the book says entities should be about identity, and all associated entities/value types can be looked up.

So to summarise:

• hidden database calls resulting in perf issues and being hard to optimise

• Attaching things to the Cargo object that increase database calls when possibly aren't necessary.

Do I get to keep my house?

The problem here is that the methods on the interface are queries masquerading as commands

In my rookie experience of DDD, I would have try to create a new detective class that will do the job of handling the found history and return a Delivery report. The cargo should not be modified here.

I think the service should still send events because scattered events is not easy to maintain.

Actually, I am probably wrong since the title of the post is about application events.

Ok, I do not know why you can find painful here....

What I find like weird is: if I were working with the IApplicationEvents is that I do not know what is the ApplicationEvent doing? I do not know if it's sending an email, writing on a log, sending a sms, post something on fbook or tweet the event.....or whatever the implementation does....

Maybe I should not know and just trust my IOC.....

That's what I find weird, just looking at the code, and not knowing anything about the implementation......

• Ok, I do not know what you found painful...

Every time a new application event is discovered as a part of requirements you're going to have to:

1. Add it to the IApplicationEvents interface
2. Create an implementation of the method for the implementors of IApplicationEvents
3. Add a new property to Cargo.Delivery (whatever that is [why does cargo know the details of it's own delivery? missing abstraction?])
4. Find every place that IApplicationEvents is used and see if it needs to be updated

IApplicationEvents does not adhere to the Open Closed Principle. Rather than centralizing and modelling business rules and processes they've been spread out all over the application and the wrong things have been pulled together (implementors of IApplicationEvents need to change every time there's a change in the way the business reacts to anything).

All they've done is moved the methods into seperate classes, made interfaces for the classes and then used injection to connect everything together. It looks like something has been achieved, but actually we're only a small step away from static global methods and procedural programming. Things appear to be more organised, but now everything is less discoverable and impossible to optimise for performance.

Again I imagine that a lot of this has been done under the mantra of TDD.

The other problem I have is with the dissonance in the naming. I would have no idea what the method "InspectCargo" is supposed to be doing. The fact that it fires domain events and updates the cargo seems wrong to me. "Inspect" means to look, not to change.

A better system would not require a class like this. As the cargo is handled, its delivery progress should be updated - i.e. the code that does the 'cargo handling' should be in charge of updating the cargo's delivery progress. Either the cargo-handling code or the cargo object itself should then fire the 'application events'.

applicationEvents doesn't need to be an interface. Having it as an interface bloats the code base and adds no value (could instead use class with virtual methods). I'm guessing that is what is bugging you.

Now here's some things that bug me.

Validate.NotNull(trackingId, "Tracking ID is required"); //useless

logger.Warn("Can't inspect non-existing cargo " + trackingId); /when is this ever useful?/

return; /something potentially horrible happened and the only person who knows this is the person reading a warning in a log file on his spare time/

HandlingHistory handlingHistory = handlingEventRepository.LookupHandlingHistoryOfCargo(trackingId); cargo.DeriveDeliveryProgress(handlingHistory);

Some DDD practitioners do this as a rule. They pass data into cargo instead of letting cargo retrieve the data it needs to "derive progress". This often leads to code that performs poorly and potentially leaks business logic into boundary classes.

Mike Minutillo, I didn't look at what the application events are doing in this sample. So I am not sure if they are used incorrectly. But I have often seen people abuse domain events which leads to the problems you describe.

Domain events in CQRS are great. Raising events so your persistence infrastructure can do it's job or let other subsystems handle events they are interested in is good design. But it's usually a very poor design when I see domain events raised and handled within one subsystem with each event handler performing business logic, Most of these times the events could instead be synchronized from one controller class which would be much more readable/maintainable.

The most obvious problem of this code is that it does business and data access together and thus is pretty damn hard to test. Drop the service and the repository, access the ORM directly to load aggregates, put everything in the controller/application.

You should NEVER communicate directly between aggregate (and thus create coupling), the plumbing (Events/Messages/Service/return value) should be there for that purpose.

Here, a simple change (in addition to removing the useless abstractions of service and repo)

var cargoItinerary = orm.Load<CargoItinerary>(trackingId); var progress = cargoItinerary.DeriveProgress(); // no writes, assuming we don't need to save the progress results

switch (progress) // do event plumbing.

// Then make the Cargo listen to this event with plumbing

OnCargoMisdirected(trackingid) { var cargo = load(trackingid); cargo.SetCurrentProgress(misdirected); store(cargo); };

This is far from perfect but much better. UTs are:

Test_Itinerary_Derive_Progress_CaseX { var itinerary = new fakeItineraryCaseX; var progress = itinerary.DeriveProgress(); assert(progress); }

Test_Cargo_Set_Current_Progress_CaseY { ... }

2 really simple set of UTs that relates directly to the business at end. Plumbing can be tested in integration tests or in specific plumbing tests.

Don't know if that's what you meant, but i see a problem if the application has/going to have a feature for inspecting multiple cargos. If so, calling this method in a loop will cause 2N queries (one per cargo and one for the handling history of each).

If the feature isn't there yet, at least from my experience, such an architecture might cause programmers to just call that method for each tracking id, not noticing the performance implications.

The main problem for me looks the fact that the implementation of service calls events, but these events are nowhere to see in the service. The service interface as it is defined is incomplete and you must know a lot of implementation details to know that it fires events. There is too little responsibility in the service.

Just thinking out loud:

... methods, interface names seem to be wrong, it is not very clear what they are expected to do. IApplicationEvents is probably ICargoTrackingEvent but maybe not because it looks like a mix of everything. ... feels messy. ... and why it has 's' at the end?

If I want to be notified that cargo has arrived (just to one event), I will have to implement all these interface methods.

... ReceivedHandlingEventRegistrationAttempt ... I forgot the beginning when I reached the end of this method name :). ... and what it has to do with Cargo ... or Application? ... I get notified when there is an attempt to handle ... brrrrr.

I find neither interface to be useful.

I can't see what ICargoInspectionService could be an abstraction for. The name "CargoInspectionServiceImpl" (exact same name as the interface with an additional "impl") indicates us it is likely to be the only concrete implementation. Except another one for the unit tests, maybe.

As for IApplicationEvents, it has such a misleading name that the real intent behind it remains unclear. I mean, it only contains Cargo-related methods but the name doesn't even have "Cargo" in it. This would hint at a God class containing every single domain event (ugh). Again, if that class contents itself with raising events, I can't see another implementation than that simple, straightforward one.

Why doesn't CargoInspectionService fire the events itself ? After all, it is the job of a good inspector to issue the appropriate warnings !

Ayende, welcome your thoughts on a slightly different design paradigm called State Driven Design (SDD). Its still in its experimental stages.

Here's the link to the article on SDD: http://sonymathew.blogspot.com/2012/01/state-driven-design-sdd.html

(apologize if this is my 2nd comment, not sure the the 1st succeeded).

You inspect the state of the cargo and fire some events manually. It'd be cleaner to use the anemic domain antipattern. It'd be cleaner to use domain events.

The way this is structured is a guarantee for code duplication and forgotten events. Ugh.

It's also going to be problematic if that code path doesn't always set those values, and sometimes leaves the existing ones in place somehow -- you'll end up posting events that don't necessarily make sense.

I think you have found an inferior implementation of the classic DDD Shipping Context. Still, I think you might do better not using "abstraction" here. The very definition of Domain Model is "a system of abstractions." The abstractions provided by the model are imperative, while the use of interfaces is not.

The cargo object is an invalid state before cargo.DeriveDeliveryProgress(handlingHistory) is called. Also, as others have said the "events" implementation is clunky.

Does Cargo do anything? Give that thing some purpose in life.

Apart from naming I have the following points:

• low cohesion: does the cargo class have some logic apart from stupid getters and setters
• IApplicationEvents contains about 3 events which are related to the ICargoInspectionService only, so why not keep it there ? This abstraction is somehow artificial
• I need to perform two different lookups to get the cargo with its history, that could be done by one query

...

Hi, I was deepy involved with the creation of the DDDSample application (I wrote most of the actual code).I believe that a few things need to be clarified here.

DDD is about managing the complexity of the domain. This is done by employing strategic design (bounded contexts etc) and tactical design (aggregates, entities, various services etc) to create models that are useful for solving a particular problem, and that are expressed in a language that is shared between developers, domain experts and code (the ubiquitous language). The purpose of the DDDSample application is to embed a rich domain model in layers of realistic technical infrastructure, surrounded by other example contexts that is has to have strategies for interacting with. It is over-engineered to provide a larger number of design examples.

Extracting a handful of interfaces from the application and presentation layers and calling the application prodcedural is just wrong. That ignores the very heart of the application, where almost all of the interesting domain-related stuff is handled. Now, I understand that the blog series is about events (more about that later) and that's all well and good, but as a way to understand the DDDSample application itself it is next to useless, and will make people wonder whether Cargo "does anything". For a quick glance at what the domain model is capable of, have a look at the scenario tests:

https://dddsample.svn.sourceforge.net/svnroot/dddsample/trunk/dddsample/tracking/core/src/test/java/se/citerus/dddsample/tracking/core/domain/scenario/CargoLifecycle.java

https://dddsample.svn.sourceforge.net/svnroot/dddsample/trunk/dddsample/tracking/core/src/test/java/se/citerus/dddsample/tracking/core/domain/scenario/VoyageRescheduledScenarioTest.java

The role of the Application layer (or Service layer), where most of these interfaces belong, is to handle use cases. Use cases also happen to align fairly well to technical aspects like transactions and access control. They follow the basic pattern of loading one or more entities based on some input, instructing them to perform some complex operation and then storing them back. As someone pointed out, it has to be done somewhere. What makes it a DDD approach is that the domain complexity is handled by the model, none of which is visible here.

Another important area is aggregates. The CargoInspection service is an application service whose sole purpose is to put the Cargo aggregate in synch with changes to the HandlingEvent aggregate. Application events are events that are published and consumed by the application layer. An event like "the cargo aggregate has been updated" is an application event. That is how different parts of the system communicate in an message-based way, and it is a way to actually implement a design principle such as "updates inside aggregates should be synchronous; updates between aggregates should be asynchronous".

Application events are different from domain events in this application. We are using the Evans flavor of the Domain Event pattern (http://www.domainlanguage.com/newsletter/2010-03/#domainEvents), which is a third category of model member, next to Entity and Value Object (though not in the book). It is something that represents a historical fact in the domain (the handling of a cargo), and is a persisted object with relations to other objects. It is not something that is passed over JMS.

The last couple of years a different flavor of domain event has been spreading, pioneered by (I believe) Udi Dahan (http://www.udidahan.com/2009/06/14/domain-events-salvation/). The difference is that the latter kind are message-style events and fired from inside the domain model. That is a perfectly valid approach, but it is not the approach that was used in DDDSample.

I certainly welcome critique of the application, that's kind of what it's for, but in this case people reading this have to realize that the 500 most important lines of code are overlooked. Anyone who's interested in what the actual model looks like, here's a good place to start:

https://dddsample.svn.sourceforge.net/svnroot/dddsample/trunk/dddsample/tracking/core/src/main/java/se/citerus/dddsample/tracking/core/domain/model/