I’ve been using CQRS architecture for about 4 years now on projects of different size and life expectancy and today after another refactoring session I would like to share ideas that seemed good when first done, but turned out to be a maintenance nightmare.

Don’t Inherit Handlers

A few times I’ve seen very similar commands or queries, that look like they are doing the same thing. And inheritence looked good when started implementing handlers.

One of the recent examples was command to add an absence for an employee and add a holiday for an employee. Where holiday is a type of absence – that was the original assumption.

So I’ve produced command to create absence:

public class CreateAbsenceCommand
{
    public int PersonId { get; set; }
    public DateTime Start { get; set; }
    public DateTime End { get; set; }
    public String AbsenceType  { get; set; }
    // etc.
}

and then produced command to create holiday:

public class CreateHolidayCommand : CreateAbsenceCommand
{
    public CreateHolidayCommand()
    {
        AbsenceType = "Holiday";
    }
}

And there was a single class to handle both cases:

public class CreateAbsenceCommandHandler : ICommandHandler<CreateAbsenceCommand>,
                                           ICommandHandler<CreateHolidayCommand>
{
    public void Handle(CrateAbsenceCommand command)
    {
        PrivateHandle(command)
    }

    public void Handle(CreateHolidayCommand command)
    {
        PrivateHandle((CrateAbsenceCommand)command)
    }

    private PrivateHandle(CrateAbsenceCommand command)
    {
        // do stuff
    }
}

That looked neat! I saved some code duplication! Only to later find out that Holiday had their own Type (Paid/Unpaid/etc.). So I went – not a problem, I’ll just adjust the code to handle holiday type. Here goes a small cludge to exclude holiday type from absences.

Next problem was that different people were meant to crate these records. HR approves holidays, managers record absences. And different level of access was required. So here goes another cludge into the codebase.

The last straw was a requirement going like this: if absence is of type “Sickness”, we need to know what category of sickness that is. At this point there was enough cludges in this part of codebase, I decided to abandon attempt to remove “code duplication”. And split commands and handlers as unrelated.

Turned out that these commands and handlers almost had nothing in common and by removing all the cludges I put in code became a lot simpler and much more maintainable. And about half the size, even though more classes.

Moral of the story – don’t prematurely fix “code duplication”, especially by inheritence. That applies to any types of architecture, not just CQRS.

One Handler – One Class

Going back to the previous sample, but without inheritence:

public class CreateAbsenceCommandHandler : ICommandHandler<CreateAbsenceCommand>,
                                           ICommandHandler<CreateHolidayCommand>
{
    public void Handle(CrateAbsenceCommand command)
    {
        // Create Absence
    }

    public void Handle(CreateHolidayCommand command)
    {
        // Create Holiday - unrelated to absence
    }
}

That was an attempt to save classes and put handlers for different commands into one class. This works just fine with Mediator and with most DI containers, so working code sample, even without inheritance.

Only later this will be a headache to read and understand what is happening. Just don’t do this. Classes are cheap, files are cheap. Create another file with another handler for your second command. No point in trying to save (space?) and cramming different handlers into one file/class.

A Query Per View

I’m mostly working with MVC projects. A lot of the time a page(view) needs data from a storage. If a project is of any size, chances that you’ll find a query that already gets 80% of information you need in the view. Then you’ll try to reuse that query to give you the other 20% – you’ll add some more columns, add if-statement somewhere to address the missing 20%. And you’ll work very hard to make sure the query works in the new page and does not break other places where it is used. And everything will be fine.

Only 6 months down the line you will get a change request to modify the other page where your query was used in the first page. And you will not remember why you’ve done that strange if-statement and why these columns are added.

Well, just don’t reuse queries like that! Rule of a thumb – a query per view. You’ll thank me later!

If you ask about duplicating logic in different places, I’ll ask you “How sure are you that this is a duplicate?”. Same principle as with inheritance above: until you have enough evidence that this is direct duplicate, don’t collapse it into one.

Reusable Query

There are always exceptions. One of the projects I work on is an HR application with a bunch of reports. Most of the reports aggregate data related to employees – they aggregate different types of data, but employees are usually the same. Every report filters employees in some way, but displays different information related to employees: absences, training, work, etc.

After writing enough reports over the years of development we have discovered a great pattern of a reusable query: employees are always filtered in the same way, only displayed information is different by report. So to filter employees we have crated a query that returns a list of employee IDs according to applied filters:

public class EmployeeIdsQuery : IQuery<IEnumerable<int>>
{
    public int? DepartmentId { get; set; }
    public int? EmploymentTypeId { get; set; }
    public int? ContractTypeId { get; set; }
    public bool? IsAgency { get; set; }
    //etc.
}

And then this query is reused in actual report queries:

public class HolidaysReportQuery : IQuery<IEnumerable<HolidayView>>
{
    public EmployeeIdsQuery EmployeeIds { get; set; }
    public DateTime PeriodStart { get; set; }
    // etc.
}

public class HolidaysReportQueryHandler : IQueryHandler<HolidaysReportQuery, IEnumerable<HolidayView>>
{
    //...
    public IEnumerable<HolidayView> Handle(HolidaysReportQuery query)
    {
        var employeeIds = mediator.HandleQuery(query.EmployeeIds);

        //then handle holidays for given employee IDs 
    }
}

This pattern worked very well for us. But this is one example out of hundreds other queries that I wrote. In every other case trying to reuse a query, either turned out into a refactoring session (where query became One-View-One-Query) or will be some time in the future when requirements change or bug is discovered.

Queries and Commands must be Serialisable

Yesterday I came across a query that looked like this:

public class GetSomeEntityQuery : IQuery<SomeEntityView>
{
    //...
    [JsonIgnore] // so we can log the query
    public Func<String, int, String> FormattingFunc { get; set; }
}

I puzzled over this for a while, trying to understand the reasons for using a function this way and trying to find a better way of doing what it was doing.

In a nutshell this function was describing how to format an output string based on some external parameters passed as closure. [JsonIgnore] attribute on top of this property raised my eyebrows as well – we do log queries as JSON. But Func<> can’t be serialised into JSON, so we had to exclude it.

Remember the rule “One-View-One-Query”? This property-function is the cludge that allowed to reuse the query in different places. After refactoring and getting this Func<> out of the query I halfed the amount of code, made it more maintainable and reduced coupling between data retrieval and data presentation.

So as soon as you see something funky (pun intended!) like this, especially when a query or command can’t be serialised easily – gives you a huge reg flag.

Follow Your Naming Convention

We have set naming convention for commands and queries handlers like this: CommandNameHandler or QueryNameHandler. I.e. handler class name is created out of command (or query) name with Handler appended. This makes it easy to find handlers, especially when you use Resharper. Too many times I’ve seen command class renamed, but handler class not renamed – makes it a lot harder to discover, especially in a large codebase.

It also helps to put command/queries classes into the same file as handler. This gives you a slice of your logic in one place and you don’t need to hunt for it. This is not always possible though: i.e. commands defined/used in Domain layer, but handlers are in Persistence layer or on the other end of the queue.

Conclusion

Most of the problems I’ve talked about here came from premature code de-duplication, when code was actually not duplicate at all. Code looked similar, acted similar, but actually was a different thing all along. So not until you have enough evidence that the code is indeed doing the same thing – extract that into a separate component.