6
\$\begingroup\$

I've been working on our application (CQRS and DDD) for awhile now. The application architecture is well layered and thought through.

However we are having difficulties in decide where to put the below code, in such a way that it makes sense from an architectural point of view.

What the code does: It's a generic method which executes a query against the dataset, specified by the generic parameter which inherits a particular interface. Note: we are not using repositories, since the query is, as the name indicates, for our read layer.

The interface, used by the generic method:

public interface IOrganisationalDbEntry
{
    string OrganisationLevelId { get; set; }
    OrganisationLevelDbEntry OrganisationLevel { get; set; }
}

The generic method, which lives inside our Entity Framework DbContext:

public IList<TDbEntry> GetByOrganisationLevel<TDbEntry>(Guid? organisationLevelId,
    Func<TDbEntry, bool> filter = null)
    where TDbEntry : class, IOrganisationalDbEntry
{
    var organisationLevel = organisationLevelId.HasValue ? organisationLevelId.Value.ToString() : null;

    var query = filter == null ? Set<TDbEntry>() : Set<TDbEntry>().Where(filter);

    IList<TDbEntry> result = query.Where(x => x.OrganisationLevelId == organisationLevel).ToList();

    // If there are no results for the current organisation level
    // => recursive check for entries for parent organisation level
    if (!result.Any() && !string.IsNullOrWhiteSpace(organisationLevel))
    {
        // Get parentOrganisationLevel
        var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
            .Where(x => x.Id == organisationLevel)
            .Select(x => x.ParentOrganisationLevelId)
            .SingleOrDefault();

        return
            GetByOrganisationLevel<TDbEntry>(
                string.IsNullOrWhiteSpace(parentOrganisationLevelId)
                    ? (Guid?)null
                    : new Guid(parentOrganisationLevelId), filter);
    }

    return result;
}

As you may be thinking, we have several EF Entities inheriting the IOrganisationalDbEntry. What comes to mind first, is a base Repository. But We do not want to use repositories in our query side, since we want this layer to be as flat as possible.

The next thing that comes to mind is a specific query and queryhandler for this query. Tho this is not acceptable since this method accepts a Func parameter to be able to add extra filter criteria while calling this method, to make it more flexible.

At this moment, this method is living as method on our IDbContext and DbContext implementation. However we are not satisfied with this. We are wondering whether there is a specific pattern or approach for our case. Imagine we have more of these methods, I do not want to keep expanding our context. Since this is not what the DbContext should be used for.

EDIT: Everywhere you read CQRS in the above question , replace it by CQS. I didn't want to modify the question since it would make some comments irrelavant.

\$\endgroup\$
8
  • \$\begingroup\$ I'm also not sure this a great fit here. It's bordering on a design question. I would recommend Programmers, but I'm not sure it would be well received there. It seems too narrow for their scope. We can certainly review the code you've posted though. \$\endgroup\$
    – RubberDuck
    Aug 19, 2014 at 12:22
  • 2
    \$\begingroup\$ It's narrow, but on the other side it's a pretty heavy question imo. Ok, I can just create a static helper and problem fixed. But we all know we dont want that to happen. Reviewing the code is always welcome, even tho we're happy with it's current state (of the code it self, not where it should be placed) \$\endgroup\$ Aug 19, 2014 at 12:44
  • \$\begingroup\$ I'm refraining from casting a close vote since you're okay with having the code reviewed. Suggestions on where the code should be placed may or may not come, but we'll never know if the question gets closed. \$\endgroup\$
    – RubberDuck
    Aug 19, 2014 at 12:47
  • \$\begingroup\$ Ofcourse I'm ok with code review! But if this question get's closed, I hope i get any feedback on where to put this question, if even programmers isn't a fit. I'm pretty sure I'm not gonna get the desired answer on Stackoverlow itself. :( \$\endgroup\$ Aug 19, 2014 at 12:56
  • \$\begingroup\$ Moved my answer to a comment, wasn't important enough for an answer. Use IEnumerable<> instead of IList<> since you probably won't add/remove items of the result of the query \$\endgroup\$
    – IEatBagels
    Aug 19, 2014 at 14:19

3 Answers 3

3
\$\begingroup\$

I'll give it a shot since CQRS is something I'm still learning.

In CQRS we are supposed to separate our read model from our write model. Is this what you have done?

Then the read model should be designed to be a perfect fit for the thing that needs to read it:

  • Your query looks very complex. Is it derived from the write model, or do you indeed have separate tables for the queries?
  • Why a Func as a parameter? It seems strange to me that the criteria should be a negotiation between client and query.
  • Why do you put the query method onto the DbContext? That class will soon enough be cluttered with too many queries.

Here are some suggestions to simplify things:

  • Let the DbContext mimic the database tables only and nothing else. That means your DbContext will only have public properties for the tables you are going to query.
  • Don't call it a Repository, but a Query. Split the Query-class when it grows too big.
  • Let the Query class take the DbContext as a dependency. Query the DbContext-properties, don't use Set<T>.
  • Let each query-method accept one criteria object as parameter, then build the query expression from that inside the method.
  • Linq is tricky. While not entirely true, a rule of thumb is that a Func runs client side, while an Expression runs server side. That's also why I don't like the Func-parameter.
  • Consider using only non-generic query-methods. Yes, there will be some duplication, but you want your peers and the future you to understand the queries two months from now.

I meant to post some code examples, but Chrome crashed again and edits got lost :-(

\$\endgroup\$
2
  • \$\begingroup\$ We don't have seperate read models for everything, so you could call it CQS instead of CQRS at some points in the applications. So basicly the issue is not valid in the CQRS context, but it is in the CQS context. The reason for a Func is that this is not a query, it is a helper used by several queries. Set<T> is neccesary in this case. We have several objects in our database which are not designed for our read model. They both share a common interface (Hence, T : IOrganisationalDbEntry). The rest of you answer is explaining how Queries should look like, which we have covered. \$\endgroup\$ Oct 6, 2015 at 14:43
  • \$\begingroup\$ Whether we should use Func vs Expression depends: fascinatedwithsoftware.com/blog/post/2012/01/10/… I started of with an Expression, but modified it to a Func. \$\endgroup\$ Oct 6, 2015 at 14:55
3
\$\begingroup\$
var organisationLevel = organisationLevelId.HasValue ? organisationLevelId.Value.ToString() : null;

var query = filter == null ? Set<TDbEntry>() : Set<TDbEntry>().Where(filter);

IList<TDbEntry> result = query.Where(x => x.OrganisationLevelId == organisationLevel).ToList();

Using C#6, you could change the first line to :

var organisationLevel = organisationLevelId?.Value.ToString();

The ?. operator will return null instead of throwing a NullReferenceException if !organisationLevelId.HasValue.

The two following lines could be changed to this :

var query = Set<TDbEntry>().Where(x => x.OrganisationLevelId == organisationLevel);

if(filter != null)
    query = query.Where(filter);

You could argue it's a useless change, but I think it's much clearer without the ? operator.

Since your result is casted to a IList<>, you should use .Count > 0 instead of !.Any() here :

if (!result.Any() && !string.IsNullOrWhiteSpace(organisationLevel))

It's more performant because Count is a pre-computed value, whereas Any() needs to call the iterator and check if there's a value.

This :

    var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
        .Where(x => x.Id == organisationLevel)
        .Select(x => x.ParentOrganisationLevelId)
        .SingleOrDefault();

Could be changed to :

    var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
        .Select(x => x.ParentOrganisationLevelId)
        .SingleOrDefault(x => x.Id == organisationLevel);

I'm not sure there's an impact on the SQL generated query (you should check with the profiler), but it removes a line of code and is in my opinion better!

Btw, you shouldn't use var here. I can't figure what's the type of parentOrganisationLevelId because of it. The guideline is to use var only when you can figure the type by reading the code, which isn't the case here.

return
    GetByOrganisationLevel<TDbEntry>(
        string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? (Guid?)null
            : new Guid(parentOrganisationLevelId), filter);

That is kinda disgusting. First, the indentation (I'm guessing Resharper placed it like that? It always does this to me) isn't cool. I understand that the ternary operator (?) is good to be split on multiple line, it's clearer. But I really think the Guid? definition should be moved to a variable declaration for readability.

var parentOrganisationLevelGuid = string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? (Guid?)null
            : new Guid(parentOrganisationLevelId)

return GetByOrganisationLevel<TDbEntry>(parentOrganisationLevelGuid, filter);

Btw, think about using default(Guid?) instead of (Guid?)null. It's just weird!

Last comment, to reduce nesting you might want to revert your condition :

if (result.Any() || string.IsNullOrWhiteSpace(organisationLevel))
{
    return result;
}

// Get parentOrganisationLevel
var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
    .Select(x => x.ParentOrganisationLevelId)
    .SingleOrDefault(x => x.Id == organisationLevel);

var parentOrganisationLevelGuid = string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? (Guid?)null
            : new Guid(parentOrganisationLevelId);

return GetByOrganisationLevel<TDbEntry>(parentOrganisationLevelGuid, filter);

You're using String.IsNullOrEmpty for nothing, since organisationLevel is defined by the Guid?.Value. The value will always either be a Guid.ToString() or null. So just checking for null would be good.

These are all readability issues. I really think your code is more readable this way :

var organisationLevel = organisationLevelId?.Value.ToString();
var query = Set<TDbEntry>().Where(x => x.OrganisationLevelId == organisationLevel);

if (filter != null)
    query = query.Where(filter);

var result = query.ToList();

if(result.Count > 0 || organisationLevel == null)
{
    return result;
}

var parentOrganisationLevelId = Set<OrganisationLevelDbEntry>()
    .Select(x => x.ParentOrganisationLevelId)
    .SingleOrDefault(x => x.Id == organisationLevel);

var parentOrganisationLevelGuid = string.IsNullOrWhiteSpace(parentOrganisationLevelId)
            ? default(Guid?)
            : new Guid(parentOrganisationLevelId);

return GetByOrganisationLevel<TDbEntry>(parentOrganisationLevelGuid, filter);

I stripped the comments because they aren't clear. You should explain why you do the recursive check with the parent...Id instead or stating you do a recursive call. We can see that by ourselves! :)

Finally, does string.IsNullOrWhiteSpace(parentOrganisationLevelId) implies you could have whitespaced IDs in your database??!?!? If so you have so many problems bigger than.. every other problems you might have. That'd be the biggest problem ever.

PS : I already wrote a comment on your question, but you should return IEnumerable<> instead of IList<>!

Warning!! (lol)

The following is rated M for mature isn't about code review but about OP's question which is where this code should belong.

I realize there was a question hidden under all that text :p You're wondering where to place this method. Let's see our options :

  • The repository/DataContext : Bad plan, it's too specific to be in a repository in my opinion.
  • Your application services/Controller : Bad plan, it's too data-oriented.
  • Some random class : That sucks, period.

What you need, I believe, is a QueryService (the name could be changed, that's what I'll use to talk). The query service is somewhere between your application services (or controller, depending on the complexity of your application) and your data access layer. This service will hold all specific queries that are used by the application services (or to show data to a GUI).

I can't draw here, but in CQRS (or CQS, doesn't matter in that case), you have a separation between the read and write.

When you write in your application, the process goes something like this :

GUI -> Application Services -> Domain -> DAL

Now, a common mistake is to mix the Read path with the Write path. What is just above is made for multiple purposes : Catching user intent, validating it through the domain and saving it in a DB. This process is captured in the 4 layers I wrote above.

When you read, none of the purposes I wrote above apply. You usually want to do A : Validation, which requires a database read or B : Expose data to your user via a GUI.

What does that mean? I means a different path of course!

GUI -> Query(?) Services -> DAL

The query service would usually return Read Models, or primitive types.

Hence, I believe you should have read services.

Application service definition : If your application has little complexity, this is probably mixed in your controller, otherwise you probably have classes that take care of validating/parsing/saving/etc. input from the GUI, that's my definition of an application service!

\$\endgroup\$
6
  • \$\begingroup\$ I do agree with most of your comments, but none of them answers my question.... We don't use C#6, we are using VS2010 (I know, this is ridiculous). Returning a value halfway of a function is considered bad practise. People hate or love it. But we don't do it. The Identation is a guideline we use inside our team, not something resharper "just" does. And no, we don't have whitespaced ID's, we have whitespace'd foreignkeys (hence the parent in parentOrganisationLevelId). Our keys are CHAR(36) btw (god forbid, I know). \$\endgroup\$ Nov 5, 2015 at 16:13
  • \$\begingroup\$ You are simply improving the code, which is always a good thing and a bit the reason of "codereview". But none of it answers my question, which is the part I rly like to see reviewed :( However, the effort in improving code quality is ALWAYS appreciated. Thanks ! \$\endgroup\$ Nov 5, 2015 at 16:14
  • \$\begingroup\$ @FrederikP. I'll add an edit for your question \$\endgroup\$
    – IEatBagels
    Nov 5, 2015 at 16:19
  • \$\begingroup\$ Lovely, I'll have a look at it and accept it if it satisfies me! You're remarks are very much appreciated! :-) \$\endgroup\$ Nov 5, 2015 at 16:23
  • 1
    \$\begingroup\$ @FrederikP. reading your question However we are having difficulties in decide where to put the below code, in such a way that it makes sense from an architectural point of view. I would guess that should be better asked on programmers.se but make sure to read their help pages first. \$\endgroup\$
    – Heslacher
    Nov 5, 2015 at 16:37
0
\$\begingroup\$

I agree with your statement that creating a new query and query handler for this is not acceptable. But it's not because of the Func parameter. It's because you've already created a query, or, to be more specific, you've used generics to create a--wait for it--generic query.

Wild guess here, but I'm thinking your architecture probably has interfaces like IQuery and IQueryHandler in it, so that all of your concepts like "query" and "query handler" follow a specific type structure. Also, all queries must be handled by a corresponding query handler. If this is the case, it's understandable why this object is incompatible with your existing "queries." It is okay for something to be conceptually a query while not being constrained to type TQuery.

If that is not the case, I still get the impression that there is some general (possibly dogma-inspired) sticking point where it "doesn't fit."

My recommendations:

  1. Turn your method into a generic class where the type parameter is defined on the object, not a method. Your naming can stay the same; it is totally acceptable for a query object to be named that way.

  2. Like Thomas Eyde suggests, pass the context as a dependency. It's going to be all right.

Either:

3a. Pass the Func into the constructor, enforcing the idea that every query has its own filter.

3b. (I recommend this) Completely remove the Func filter but defer the execution of the query by returning an IQueryable instead of an IList<T>. You can then chain the filter on instead of having to pass an expression or delegate to a method (usually a code smell if you aren't writing functional code or dealing with events).

I think the IQueryable is the way to go because it allows you to keep the filtering out at the consumer level and doesn't eagerly pull all the records back.

Notes on the recursion:

  • You're essentially performing a recursive self join on a table. Why not simplify everything by using a stored procedure with a CTE? Obviously there are some EF considerations here, but it could be a worthwhile endeavor.
  • If you take the IQueryable approach obviously you will have to refactor the code to remove the subsequent lookups. Naturally if you do this you will have considerations around how to create an object that can be called generically for everything that derives from your interface. However, if that becomes a grand undertaking, it's not a limitation of EF, or the framework, it's a complication of a design issue. Consider what you have going on currently: a recursive call that makes a round trip every iteration. Usually this is a sign that things can be done better.

Things to avoid getting hung up on:

  • You can directly instantiate this class. It and all its dependencies don't have to be injected.
  • Don't require a handler for this query. You want to keep the layer as flat as possible, right?
  • Unit testing can be achieved via the LINQ to Objects provider which doesn't necessarily always mirror the EF provider. Okay, that's something to watch out for, but how many tests do you have around this already?

In scenarios where code has been written and there's a question of "now where does it go," it's usually a good idea to question if the overall mindset is "don't break rules" or "try to align with the spirit of the pattern." It's only when you fully invest in the latter that you find the freedom to try the techniques that will eventually lead to a good solution.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.