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!
IEnumerable<>
instead ofIList<>
since you probably won't add/remove items of the result of the query \$\endgroup\$