Simplifying code with Decorated Commands by Mark Seemann
Consider modelling many side effects as a single Command.
In a previous article I discussed how an abstraction can sometimes be leaky by omission. In this article, you'll see how removing the leak enables some beneficial refactoring. I'm going to assume that you've read the previous article.
The code shown here is part of the sample code base that accompanies my book Code That Fits in Your Head.
The relative cost of the four CRUD operations #
In this article, you'll see code that implements an ASP.NET Controller action. It enables a REST client to update an existing reservation via a PUT request.
I chose to show you the Put method because it's the worst, and thereby the one where refactoring is most warranted. This seems to follow a pattern I've noticed over the years: data updates are always the worst.
Before I show you the code, I'd like to take a little detour to discuss this observation.
Consider the four CRUD operations. Which one is the easiest to implement and maintain, and which one gives you most grief?
Deletes are typically straightforward: A unique identifier is all it takes. The only small complication you may have to consider is idempotence. If you delete an entity once, it's gone. What should happen if you 'delete' it again? To be clear, I don't consider this a trick question. A delete operation should be idempotent, but sometimes, depending on the underlying storage technology, you may have to write a few lines of code to make that happen.
Reads are a little more complicated. I'm actually not sure if reads are more or less involved than create operations. The complexity is probably about the same. Reading a single document from a document database is easy, as is reading a single row from a database. Relational databases can make this a bit harder when you have to join tables, but when you get the hang of it, it's not that hard.
Create operations tend to be about as easy or difficult as reads. Adding a new document to a document database or BLOB storage is easy. Adding a complex entity with foreign key relationships in a relational database is a bit more complicated, but still doable.
Updates, though, are evil. In a document database, it may be easy enough if you can just replace the document wholesale. Often, however, updates involves delta detection. Particularly in databases, when foreign keys are involved, you may have to recursively track down all the related rows and either update those as well, or delete and recreate them.
As you'll see in the upcoming code example, an update typically also involves complicated auxiliary logic to determine what changed, and how to react to it.
For that reason, if possible, I prefer modelling data without supporting updates. Create/read/delete is fine, but if you don't support updates, you may not need deletes either. There's a reason I like Event Sourcing.
A complicated Put method #
My restaurant reservation API included this method that enabled REST clients to update reservations:
[HttpPut("restaurants/{restaurantId}/reservations/{id}")] public async Task<ActionResult> Put( int restaurantId, string id, ReservationDto dto) { if (dto is null) throw new ArgumentNullException(nameof(dto)); if (!Guid.TryParse(id, out var rid)) return new NotFoundResult(); Reservation? reservation = dto.Validate(rid); if (reservation is null) return new BadRequestResult(); var restaurant = await RestaurantDatabase .GetRestaurant(restaurantId).ConfigureAwait(false); if (restaurant is null) return new NotFoundResult(); return await TryUpdate(restaurant, reservation).ConfigureAwait(false); }
Had I written this code exclusively for myself, I'd written in a more functional style, as an impureim sandwich. (Actually, had I written this code exclusively for myself, I'd written it in F# or Haskell.) This code, however, is written for another audience, so I didn't want to assume that the reader knows about impureim sandwiches.
I still wanted to decompose the functionality into small blocks. There's still an echo of the impureim sandwich architecture in the Put method, because it handles most of the impure preparation - the top of the sandwich, so to speak.
The rest - any functional core there might be, as well as impure post-processing - it delegates to the TryUpdate method.
TryUpdate #
Here's the TryUpdate method:
private async Task<ActionResult> TryUpdate( Restaurant restaurant, Reservation reservation) { using var scope = new TransactionScope( TransactionScopeAsyncFlowOption.Enabled); var existing = await Repository.ReadReservation(reservation.Id) .ConfigureAwait(false); if (existing is null) return new NotFoundResult(); var ok = await WillAcceptUpdate(restaurant, reservation) .ConfigureAwait(false); if (!ok) return NoTables500InternalServerError(); await Update(restaurant, reservation, existing) .ConfigureAwait(false); scope.Complete(); return new OkObjectResult(reservation.ToDto()); }
To be honest, this is mostly just more impure pre-processing. The functional core is hidden away inside the (impure) WillAcceptUpdate method, but I'm not going to show you that one. It's not important in this context.
If, however, the method decides that the update is possible, it'll make one more delegation, to the Update method.
I admit it: This isn't the prettiest code I've ever written. I did warn you, though. I chose this method as an example because it could really do with some refactoring. One problem I have with it is the naming. You have a Put method, which calls a TryUpdate method, which again calls an Update method.
Even though the Try prefix is a .NET idiom, I still feel that a regular reader could be easily confused, having to choose between TryUpdate and Update.
Still, let's soldier on and review the Update method as well. It's the last one, I promise.
Update #
Here's the Update method:
private async Task Update( Restaurant restaurant, Reservation reservation, Reservation existing) { if (existing.Email != reservation.Email) await PostOffice .EmailReservationUpdating(restaurant.Id, existing) .ConfigureAwait(false); await Repository.Update(reservation).ConfigureAwait(false); await PostOffice .EmailReservationUpdated(restaurant.Id, reservation) .ConfigureAwait(false); }
The method perfectly illustrates what I meant when I wrote that you often have to do various kinds of delta analysis when implementing an update - even if delta analysis isn't required by the data store.
This method does two things:
- It sends emails
- It updates the repository
Update sends an email to the old address. This is an example of delta analysis. This only happens on a changing email address. It doesn't happen if the name or quantity changes.
The motivation is that it may serve to warn the user if someone tries to change the reservation. Only when the email address changes is it necessary to send an email to the old address.
In all cases, the method sends an email to the 'current' address.
This seems ripe for refactoring.
Plugging the leak #
The Update method is an asynchronous Command. It exclusively produces side effects, but it doesn't return anything (we'll regard Task as 'asynchronous unit').
I've known since 2011 that Commands are composable. Later, I also figured out the fundamental reason for that.
The Update method composes three other Commands - one conditional and two unconditional. This seems to call for some sort of composition: Chain of Responsibility, Decorator, or Composite. Common to these patterns, however, is that the object that they compose must share an API. In a language like C# it means that they must share a polymorphic type.
Which type might that be? Let's list the three method signatures in action, one after the other:
-
Task EmailReservationUpdating(int restaurantId, Reservation reservation) -
Task Update(Reservation reservation) -
Task EmailReservationUpdated(int restaurantId, Reservation reservation)
The commonality might be easier to spot if we X out the names (which are only skin-deep, anyway):
-
Task Xxx(int restaurantId, Reservation reservation) -
Task Xxx( Reservation reservation) -
Task Xxx(int restaurantId, Reservation reservation)
The only deviation is that the middle method (originally the Update method) lacks a restaurantId parameter.
As the previous article explained, though, this is a leaky abstraction by omission. Will plugging the leak enable a refactoring?
Let's try. Make restaurantId a parameter for all methods defined by the interface:
public interface IReservationsRepository { Task Create(int restaurantId, Reservation reservation); Task<IReadOnlyCollection<Reservation>> ReadReservations( int restaurantId, DateTime min, DateTime max); Task<Reservation?> ReadReservation(int restaurantId, Guid id); Task Update(int restaurantId, Reservation reservation); Task Delete(int restaurantId, Guid id); }
This is the suggested remedy from the previous article, so I put it here solely as a reminder.
An emailing Decorator #
There's a sequence to the actions in the Update method:
- It emails the old address about a changing address
- It updates the reservation
- It emails the current address about the update
IReservationsRepository:
public class EmailingReservationsRepository : IReservationsRepository { public EmailingReservationsRepository( IPostOffice postOffice, IReservationsRepository inner) { PostOffice = postOffice; Inner = inner; } public IPostOffice PostOffice { get; } public IReservationsRepository Inner { get; } public async Task Update(int restaurantId, Reservation reservation) { if (reservation is null) throw new ArgumentNullException(nameof(reservation)); var existing = await Inner.ReadReservation(restaurantId, reservation.Id) .ConfigureAwait(false); if (existing is { } && existing.Email != reservation.Email) await PostOffice .EmailReservationUpdating(restaurantId, existing) .ConfigureAwait(false); await Inner.Update(restaurantId, reservation) .ConfigureAwait(false); await PostOffice.EmailReservationUpdated(restaurantId, reservation) .ConfigureAwait(false); } // Other members go here... }
You may think that it seems odd to have a 'repository' that also sends emails. I think that this is mostly an artefact of unfortunate naming. Perhaps a follow-up change should be to rename both the interface and the Controller's Repository property. I'm open to suggestions, but for now, I'll leave the names as they are.
If you're still not convinced, consider an alternative architecture based on asynchronous message passing (e.g. CQRS). In such architectures, you'd put Commands on a message bus and think no more of it. A background process would then asynchronously perform all the actions, including sending emails and updating the data store. I think that people used to that kind of architecture wouldn't bat an eyelid by bus.Send(new UpdateReservation(/**/)).
This would also be close to the kind of design that Steven van Deursen and I describe in chapter 10 of our book.
Simplification #
This greatly simplifies things. The above Update method now becomes redundant and can be deleted. Instead, TryUpdate can now directly call Repository.Update:
private async Task<ActionResult> TryUpdate( Restaurant restaurant, Reservation reservation) { using var scope = new TransactionScope( TransactionScopeAsyncFlowOption.Enabled); var existing = await Repository .ReadReservation(restaurant.Id, reservation.Id) .ConfigureAwait(false); if (existing is null) return new NotFoundResult(); var ok = await WillAcceptUpdate(restaurant, reservation) .ConfigureAwait(false); if (!ok) return NoTables500InternalServerError(); await Repository.Update(restaurant.Id, reservation) .ConfigureAwait(false); scope.Complete(); return new OkObjectResult(reservation.ToDto()); }
This also means that you can remove the PostOffice dependency from the Controller. Lots of things becomes simpler by this refactoring. It better separates concerns, so tests become simpler as well.
Conclusion #
You can simplify code by composing Commands. Candidate patterns for this are Chain of Responsibility, Decorator, and Composite. These patterns, however, require a common polymorphic type. Key to refactoring to these patterns is to identify such a common interface. In this article, I used the refactored IReservationsRepository interface.
Whenever a client calls a method on the repository, a change of state now automatically also sends emails. The client doesn't have to worry about that.
Consider modelling many related side-effects as a single composed Command.
