Modify Command object inside decorator of CommandHandler(CQRS) or any other practice for avoiding duplication

Question

Currently, we have multiple commands which share two same properties (BookName and ShopId). While handling those commands those properties must be validated with the help of some service. Let's say that those validating process is just a two lines of code. Like this:

// Get book by name
var book = await _booksRepository.GetBookByName(command.BookName);

// Check whether that store contains that book
_bookValidator.ThrowIfBookIsNotStockInStore(command.StoreId, book.Stores);

And then, inside all command handlers we are using that book variable. We don't want to live with that two lines of code duplication across all command handlers.

So, one guy in our team offered to create some BaseCommand which has PopulatedBook property. And to register let's say PopulateBookCommandHandlerDecorator for required command handlers. And that decorator will Execute that two lines of code and populate Book property of the command. And of course all required commands must be derived from that BaseCommand.

In such way, we will convey getting and validating Book object to the decorator and duplication will go to the hell.

But, I think that such design smells fragility which is one of the symptoms which has to be avoided. I see two main problem here:

1. We are modifying command object during handling with the help of decorator. I think it has to be "%immutable%".
2. Decorator must be used only for cross-cutting concerns. And Getting book object from repository and updating command object is a core concern.

What you guys do think? My opinion is to live with that 2 lines of code duplication and if lines number will reach to 3 or 4 then we can introduce some injection like IBookPreparer which will validate book object and return.

Answer 1

Regarding modifying command: I would create commands and only handle it. I don't prefer to modify commands once created. (I am okay with the command handler creating a new command but modifying doesn't seem good to me)

Now coming to the problem of duplicate code: There is two line of duplicate code occurring in many places. This is not the best thing. I would prefer to create a service that could contain these two lines of code. Now in my command handler I will prefer to call one line instead of 2 (the method containing these two line). (As you said if the number of lines increases in future, this will take care).

What you colleague did is reused some code by inheritance. What I am suggesting is reuse code by composition (containment). It would be preferable to choose containment over inheritance. The reasons for this are 1. Containment is looser coupling than inheritance 2. At run time, you can change what you contain, but can not change what you are inheriting from 3. You can contain multiple things but you can not inherit from multiple things (for java and c#)

I think the idea of adding a decorator and doing something there increase the complexity also. I would prefer to KISS (keep it simple and stupid) it.

Also the BaseCommand name looks a bit lame. It clearly tells that you created this command only to inherit from it. It doesn't follow the the is a relation to well with a the commands

What you colleague is solving is DRY. If we keep the complexity low while solving it and introduce no other problems, then why not.

About the design smell "fragility". You say the code is fragile, when you do one thing and code breaks at another completely unexpected place. In this case I don't see any reason why the code is fragile.

Answer 2

I agree with Nachiappan Kumarappan and I would also suggest that you avoid inheritance and prefer composition for this type of concerns.

You need to ask yourself, first of all, why you are worrying, i.e. what reasons are your worries motivated by. While you worry about duplication, the true cause you are worrying about is probably the extensibility of your code (and I am worrying that I have used "worry" 6 times in 2 sentences...).

Today you validate this way, but tomorrow you may need to change your validation/retrieval. So you definitely need to "depend" on something, which means introduce a new parameter to the constructor, and let this external "device" perform these two lines.

Then... you consider creating a query with a handler. This is also not the way to go, but the question is, why? The answer is primarily concerned with semantics ("neatness", if you will). Command/Query handlers are generally used to (among other reasons) introduce a semantic decoupling between the Application Layer and the Domain Layer. An easy way to think about it is as: "Mr. Application, let me know what you wish to do and I will handle it for you, I will speak to Mr. Domain, I will manipulate what is needed and make it all work".

Now think about it, does it make sense to create a command/query with a corresponding handler, if you are not going to ever use it in the application layer? This validation/retrieval (the, currently two, lines), is this relevant to the application as a specific expression you need to care about in the Application Layer? If this is not a reasonable standalone query and you are almost certain that it is never going to be used in isolation (or even uncertain), then you better create a "low-level" interface-with-implementation service and use this to abstract the validation/retrieval. There is no reason to carry a query/query-handler pair around without ever using it in its proper-place (i.e. only within command handlers and not inside the Application Layer code).

If, in the future, a case comes up, in which you suddenly need this validation/retrieval all by itself inside your Application Layer and it starts to make sense in at least one use-case, you can always create a query/query-handler pair then, wrapping this service, and use it inside.

In short, create a class that executes these two lines through a method, hide it behind an interface, pass this to the constructor of the command handlers that need it and avoid inheritance because, all too often, inheritance hides intention (because you have to search around to understand what and why). While duplication is bad, not all duplication is created equal! Making the same dependency explicit in all constructors of the handlers that need it is a kind of duplication, too. However, it helps to identify the dependency easily, almost at first sight. To put it in perspective, the benefits outweigh the disadvantages.

Remember, writing good OOP code is about expressing your intention and making it as explicit as possible, so that future readers (and your future self) can know precisely what you had in mind when composing the code.