Both of these statements should go into Cargo.DeriveDeliveryProcess:

if (cargo.Delivery.Misdirected)
{
  applicationEvents.CargoWasMisdirected(cargo);
}

if (cargo.Delivery.UnloadedAtDestination)
{
  applicationEvents.CargoHasArrived(cargo);
}

I wrote in comments for last post. There should be an event class hierarcy, and factory to create them.

I'd have DeriveDeliveryProgress return a DeliveryStatusNotification, with there being sub-classes for UnloadedAtDestinationNotification, MisdirectedDeliveryNotification and so on. Each subclass would know which event(s) should be raised. The IApplicationEvents would just have a method telling it to raise the IEvent it was passed rather than have a different method for every event. The DeliveryStatusNotification would have a method RaiseEvents that accepted the IApplicationEvents, the implementation creating the appropriate events and passing them to the IApplicationEvents.

This way you can add new DeliveryStatusNotification derived classes without changing the service or the IApplicationEvents interface and you'd also be able to separate the responsibility of knowing what event should be raised from the responsibility of how/when events are propagated.

The service would look like:

HandlingHistory handlingHistory = handlingEventRepository.LookupHandlingHistoryOfCargo(trackingId);

DeliveryStatusNotification  notification = cargo.DeriveDeliveryProgress(handlingHistory);

notification.RaiseEvents(applicationEvents);

cargoRepository.Store(cargo);

I would address this by creating an Interface for Invoking events in your application. public IApplicationEventInvoker { void InvokeEvent(IApplicationEvent); }

Create a higher order event that gets invoked after the cargo has been inspected which will let you change exectuion Order

public IApplicationEvent { void Invoke(); }

public ICargoInspectedEvent:IApplicationEvent();

public CargoInspectedEvent:ICargoInspectedEvent { private cargo; private IApplicationEVentInvoker eventInvoker; CargoInspectedEvent(Cargo cargo, IApplicationEventInvoker eventInvoker) { if(cargo == null) { throw new ArgumentNullException('cargo'); } if(eventInvoker == nUll) { throw new ArgumentNullException('eventInvoker'); } this.cargo = cargo;

this.eventInvoker = eventInvoker;

} void Invoke() { if(cargo != null) { if(cargo.Delivery.Misdirected) { eventInvoker.InvokeEvent(new CargoMisdirectedEvent(cargo)); }

    if(cargo.Delivery.UnloadedAtDestination)
    {

eventInvoker.InvokeEvent(new CargoHasArrivedEvent(cargo));
    }
}
else
{
//Handle the Null cargo situation possibly invoke another event.
}

} }

Then your implmentation of the ICargoInspectionService will Invoke the CagroInspectedEvent when it is done inspecting cargo

public class CargoInspectionServiceImpl : ICargoInspectionService { // code redacted for simplicity

private IApplicationEventInvoker eventInvoker; //came in from constructor

public override void InspectCargo(TrackingId trackingId) { Validate.NotNull(trackingId, "Tracking ID is required");

Cargo cargo = cargoRepository.Find(trackingId);
if (cargo == null)
{
  logger.Warn("Can't inspect non-existing cargo " + trackingId);
  return;
}

HandlingHistory handlingHistory = handlingEventRepository.LookupHandlingHistoryOfCargo(trackingId);

eventInvoker.InvokeEvent(new CargoInspectedEvent(cargo,eventInvoker));      

cargo.DeriveDeliveryProgress(handlingHistory);


cargoRepository.Store(cargo);

} }

Now the events themselves govern the rules of what happens when they are fired, and could aslo decide to do nothing under certain conditions if they were called or call lower level events that would be conditionally invoked. This also allows other events to be created without being apart of a God interface like IApplicationEvents.

@SteveR that code looks much cleaner

I would follow what SteveR said and have DeriveDeliveryProgress return some sort of notification. Maybe something like:

IDeliveryProgress deliveryProgress = cargo.DeriveDeliveryProgress(handlingHistory);
applicationEvents.Handle(deliveryProgress, cargo);

where Handle takes in the IDeliveryProgress and does whatever kind of notification it needs to perform.

Correction: These are the statements that should go into Cargo.DeriveDeliveryProcess:

if (this.Delivery.Misdirected)
{
    applicationEvents.Raise(new CargoWasMisdirectedEvent(this));
}

if (this.Delivery.UnloadedAtDestination)
{
    applicationEvents.Raise(new CargoHasArrived(this));
}

This is an application of the very useful Domain Event pattern.

I'm a big fan of an approach I learned from Udi Dahan: http://www.udidahan.com/2008/08/25/domain-events-take-2/

This design has extremely low friction and works tremendously well.

Re: my previous comment

I pointed to the wrong article. The pattern has gone through several iterations. Here is the approach we use: http://www.udidahan.com/2009/06/14/domain-events-salvation/

We should centralize the logic inside Cargo itself using some observer implementation. The infrastructure (or individual callers) can attach contextually appropriate handlers to respond to events.

@Ayende : frankly it's a bit short for a criticism.

Are you questioning the usefulness of the Application Event pattern in this context ? The use of interfaces (as the title "limit your abstractions" seems to imply) ? The distribution of responsibilities between the objects ? The flow of events and method calls from the moment the cargo is inspected ?

You said this is problematic code "for several reasons" but what are they all exactly ?

We also painfully lack context to judge here. How big is the application ? Is it so complex or related with many other systems that it requires a complicated architecture with domain events and interfaces all over the place ? What are the other parts in the architecture ? Who subscribes to the application events and what do they do as a consequence ?

I find this code review exercise a bit absurd since we're reasoning in the abstract and on a microscopic part of a textbook app.

There's no problematic code per se. Only inappropriate code in a given context !

class Cargo { enum DeliveryStatus {MISDIRECTED, ARRIVED, UNLOADED, INSPECTED}

String trackingId; DeliveryStatus deliveryStatus; //+other properties. }

interface CargoService { Cargo read(String trackingId) boolean update(Cargo c); //+other services.. }

class CargoDeliveryStatusEvent extends Event { interface Mediator {
void fire(CargoDeliveryStatusEvent e); }

interface Listener { void handle(CargoDeliveryStatusEvent e); }

CargoDeliveryStatusEvent(Cargo.DeliveryStatus deliveryStatus) { ... } }

class CargoRepository implements CargoService { Cargo read(String trackingId) { //read from the database. }

Cargo update(Cargo cargo) { //update the database } }

class CargoBusiness implements CargoService { CargoBusiness(CargoService cargoRepository, CargoDeliveryStatusEvent.Mediator eventMediator, ...) { ... }

Cargo read(String trackingId) { Validate.NotNull(cargo.trackingId, "Tracking ID is required");
Cargo cargo = cargoRepository.read(trackingId); //do some business logic return cargo; }

Cargo update(Cargo cargo) { if (cargo == null) { throw new IllegaArgumentException("Non-existing cargo"); }

//do some business logic    

cargoRepository.update(cargo);

eventMediator.fire(new 
  CargoDeliveryStatusEvent(cargo.deliveryStatus));

}

}

class CargoClient { String trackingId = null; Cargo cargo = null;

CargoClient(CargoService cargoService, ...) { }

void onViewCargoClicked() { this.trackingId = getTrackingIdTextFieldValue(); this.cargo = cargoService.read(trackingId); showView(cargo); }

void onSubmitClicked() { updateCargoFromView(cargo); cargoService.update(cargo); } }

ouch..that looks bad, how do u format code in comments?

class Cargo {
  enum DeliveryStatus {MISDIRECTED, ARRIVED, UNLOADED, INSPECTED}

  String trackingId;
  DeliveryStatus deliveryStatus;
  //+other properties.
}

interface CargoService {
  Cargo read(String trackingId)
  boolean update(Cargo c);
  //+other services..
}

class CargoDeliveryStatusEvent extends Event {
  interface Mediator {  
    void fire(CargoDeliveryStatusEvent e);
  }
  
  interface Listener {
    void handle(CargoDeliveryStatusEvent e);
  }
  
  CargoDeliveryStatusEvent(Cargo.DeliveryStatus deliveryStatus) {
    ...
  }
}

class CargoRepository implements CargoService {
  Cargo read(String trackingId) {
    //read from the database.
  }

  Cargo update(Cargo cargo) {
    //update the database
  }
}

class CargoBusiness implements CargoService {
  CargoBusiness(CargoService cargoRepository, CargoDeliveryStatusEvent.Mediator eventMediator, ...) {
    ...
  }

  Cargo read(String trackingId) {
    Validate.NotNull(cargo.trackingId, "Tracking ID is required");    
    Cargo cargo = cargoRepository.read(trackingId);
    //do some business logic
    return cargo;
  }

  Cargo update(Cargo cargo) {
    if (cargo == null) {
      throw new IllegaArgumentException("Non-existing cargo");
    }

    //do some business logic    
    
    cargoRepository.update(cargo);
    
    eventMediator.fire(new CargoDeliveryStatusEvent(cargo.deliveryStatus));
  }
  
}

class CargoClient {
  String trackingId = null;
  Cargo cargo = null;
  
  CargoClient(CargoService cargoService, ...) {
  }
  
  void onViewCargoClicked() {
    this.trackingId = getTrackingIdTextFieldValue();
    this.cargo = cargoService.read(trackingId);
    showView(cargo);
  }
  
  void onSubmitClicked() {
    updateCargoFromView(cargo);
    cargoService.update(cargo);
  }
}

Taking a look at some sample code from Greg Young's m-r (basically the beginning of what we now call CQRS/event-sourcing) shows a much better way to implement the above logic. Like others have said, it reads way too much like Transaction Script. Procedural wrapped in OO clothing.

SteveR's code is nice, but all he's done is introduced another abstraction. Kind of ironic given the title of this series? Am I wrong?

I'm not so happy with the code presented by SteveR. It seems too easy to forget that you need to do that final step of publishing the results, and end up with stuff that's subtly wrong and hard to debug -- items aren't in a valid state; a remote service should have been called but wasn't; audit logs not being filled...

The only situation where that would make sense is if you had an explicit 'commit' phase. You call the domain objects and tell them to prepare some change; they give you a token representing the results of that change. You inspect the result token, then you decide whether to accept the change or not.

No possibility of getting into an invalid state, since you failed to commit, which means you didn't end up changing anything, which means it's okay that you forgot to raise the events...

But even then, you'd want a single 'commit' action that will raise the appropriate events. If it's in the domain objects, you're using domain events. If it's in some other service, you want the events to be raised in that service.

Anything else, you're just asking for bugs.

I think there are a couple of interesting comments made in this post. The first I think we all missed. "deciding when to execute the event is left for the caller" When should these events be called in the domain? Should the CargoWasMisdirected event be manually called after deriving the delivery progress? Really? Shouldn't this event just fire when the cargo is handled and the internal delivery status changes as a part of the shipping process, perhaps when it arrives at a new destination? I'm suggesting that this check logic is done by the setter of the Cargo's internal Delivery Status. In this scenario the application cannot explicitly raise arbitrary events. If all of the events were handled in this way, as apart of the domain behaviors, then the IApplicationEvents interface would not have any reason to even exist. The second most obvious thing that some people pointed out was the title of the post. "Limit your abstractions..." more importantly "what about change?" The IApplicationEvents interface also "violates the Open Closed Principle" because if you want to add a new event you have to change the interface and those classes implementing it. One of the major benefits of Interfaces and the Open Closed principle is the ability to get new functionality without changing your application's architecture. If you were to add a new ApplicationEvent to the interface you would not only have to change the interface file but every implementation of it. Generally interfaces should be as small as possible to avoid this problem. You can always aggregate them through inheritence, or better yet just implement more than one in a class that needs to have more than one of those behaviors. I did think SteveRs solution was interesting but like my previous post it also relies on the caller to raise the event which was the major problem Ayende was commenting on.

I wrote my proposed solution shortly after having read the original author's comment on the last post where he says that the architecture purposely makes a distinction between "Domain Events" and "Application Events", so I assume that elsewhere in the codebase there are Domain Events being raised directly from within the aggregates but here we're dealing with Application Events which (for whatever reason) don't work in that way. I haven't looked at the rest of the codebase so I'm not going to justify or defend that distinction, I was only focused on implementing what he was trying to do in a way that didn't suffer the obvious problems of the original IApplicationEvents interface.

It may be that there's no value to be derived from making the distinction between notifications and events that my solution involves, I was just following the original authors brief to show that you could maintain the distinction in a more maintainable way.

SteveR, I believe your suggested way of handling Application Events, if they must exist, is sound based on the limited information given by the code snippet, also I believe the majority of the others do agree. Yes that does definitely solve the problem of the ever changing IApplicationEvent interface. I have seen that type of thing too many times, and I'm sure at sometime I have even done it myself, fortunately we get the opportunity to learn from our mistakes. I'm really interested to see the suggested solutions in the upcoming posts.

Pub/Sub comes to mind ;)