Ayende @ Rahien

Peter Backlund commented on Limit your abstractions: Application Events–the wrong way

Mon, 06 Feb 2012 09:47:39 GMT

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/

J* commented on Limit your abstractions: Application Events–the wrong way

Mon, 06 Feb 2012 09:02:06 GMT

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 ...

Ryan Cromwell commented on Limit your abstractions: Application Events–the wrong way

Sat, 04 Feb 2012 23:44:41 GMT

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

Jay Douglass commented on Limit your abstractions: Application Events–the wrong way

Sat, 04 Feb 2012 19:46:46 GMT

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

Vaughn Vernon commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 23:20:17 GMT

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.

Chris Wright commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 22:20:22 GMT

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.

Sony Mathew commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 20:50:05 GMT

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).

Guillaume commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 19:12:09 GMT

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 !

Gena commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 18:45:35 GMT

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.

Ayende Rahien commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 18:04:25 GMT

Nick, I did enough posts about the evils of abstraction data access to last me for a month or two :-) I don't talk about _any_ data access details in this series. You do raise some valid details, but not stuff that I touch in the rest of this series. Please FedEx your house to me as soon as can be arranged :-)

Nadav commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 15:56:24 GMT

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.

Karhgath commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 14:55:30 GMT

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(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.

Giscard Girard commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 14:13:34 GMT

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.

Giscard Girard commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 13:19:50 GMT

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.

cbp commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 12:46:25 GMT

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'.

Mike Minutillo commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 12:34:01 GMT

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).

jjchiw commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 12:24:25 GMT

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

jjchiw commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 12:23:01 GMT

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......

nick commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 11:19:48 GMT

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

FX Boga commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 11:04:31 GMT

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.

Jonty commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 11:00:31 GMT

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

Nick commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 10:44:52 GMT

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?

Matt Davey commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 10:25:57 GMT

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!

Dalibor Carapic commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 10:23:35 GMT

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.

Kenny Eliasson commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 09:44:17 GMT

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

Dave commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 09:35:09 GMT

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..

Rémi BOURGAREL commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 09:23:32 GMT

@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.

gandjustas commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 09:19:40 GMT

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.

Radenko commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 09:17:59 GMT

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

Frank commented on Limit your abstractions: Application Events–the wrong way

Fri, 03 Feb 2012 08:50:49 GMT

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