5
\$\begingroup\$

I have the following code in a validation module. It works the way I want it to, but it has an obvious select n+1 problem. I would like to refactor this code to make a single call to the db to perform this validation.

Is there a way I can do it using linq or do I need to perform more major surgery? I am trying to avoid sprocs at this point, but am willing to listen to arguments for it.

private void EnsureIdentifiersAreNotInUse(AddOrEditClientCommand request)
        {
            var identifiers = request.Editor.Identifiers;

            for (var i = 0; i < identifiers.Count; i++)
            {
                var identifier = identifiers[i];
                var exists = dbContext.ClientIdentifers
                    .Any(x => x.Client.Id != request.Editor.ClientId 
                        && x.ClientIdentifierType.Id == identifier.TypeId 
                        && x.Value.Equals(identifier.Value, StringComparison.InvariantCultureIgnoreCase));
                if(exists)
                    request.ModelState.AddModelError("Identifiers[{0}].Value".FormatWith(i), "System Id ({0}) is already in use".FormatWith(identifier.Value));
            }
        }
\$\endgroup\$
  • \$\begingroup\$ Any reason why you are trying to avoid sprocs? \$\endgroup\$ – Dan Pantry Mar 10 '15 at 16:14
  • \$\begingroup\$ Hi! Welcome to Code Review. Please only state the purpose of the code in the title. \$\endgroup\$ – TheCoffeeCup Mar 10 '15 at 16:15
  • 1
    \$\begingroup\$ I would rather limit the number of sprocs in the system to ones that are actually needed. I have gone into too many codebases that have hundreds if not thousands of sprocs that nobody knows what they do or why they are even there. So as an experiment on this project, I am attempting to lean on EF for all data access. Then I want to profile the application performance and add sprocs for obvious hot spots in the code. This question is more about how can I leverage EF/linq to get the same effect without resorting to a sproc yet. \$\endgroup\$ – NotMyself Mar 10 '15 at 16:17
  • 2
    \$\begingroup\$ Hi @MannyMeng, thanks for the feedback and warm welcome. I don't quite follow your request. Can you point me to some examples or wiki guides for proper title format for this exchange? I am happy to edit. \$\endgroup\$ – NotMyself Mar 10 '15 at 16:20
  • \$\begingroup\$ @MannyMeng you are welcome to suggest edits to questions that will be reviewed by other members of code review if you think the question is not suitable in it's current state. I think that the question is fine and the extra paragraph adds necessary context. \$\endgroup\$ – Dan Pantry Mar 10 '15 at 16:23
2
\$\begingroup\$

I think this might work for you

void EnsureIdentifiersAreNotInUse(AddOrEditClientCommand request)
{
    var identifiers = request.Editor.Identifiers;

    var errors = 
        dbContext.ClientIdentifers
            .Where(ci => ci.Client.Id != request.Editor.ClientId
                   && identifiers.Any(i => ci.ClientIdentifierType.Id == i.TypeId
                                      && ci.Value.Equals(i.Value, StringComparison.InvariantCultureIgnoreCase)));
}

UPD Ok, your identifier type is not presented at db, so the query cannot be converted to a db query.

Let's try this:

void EnsureIdentifiersAreNotInUse(AddOrEditClientCommand request)
{
    var identifiers = request.Editor.Identifiers;
    var identifiersIds = identifiers.Select(i => i.TypeId).ToList();
    var identifiersValues = identifiers.Select(i => i.Value).ToList();

    var errors = 
        dbContext.ClientIdentifers
            .Where(ci => 
                   {
                       if (ci.Client.Id == request.Editor.ClientId)
                       {
                           return false;
                       }
                       var indexOfId = identifiersIds.IndexOf(ci.ClientIdentifierType.Id);
                       if (indexOfId == -1)
                       {
                           return false;
                       }
                       return indexOfId == identifiersValues.IndexOf(ci.Value);
                   });                       
}
\$\endgroup\$
  • \$\begingroup\$ Hi @aunh, I thought you were on to something here and it was so simple. Sadly I get the following exception: Unable to create a constant value of type 'UIWeb.Models.Clients.ClientEditorForm+Identifier'. Only primitive types or enumeration types are supported in this context. \$\endgroup\$ – NotMyself Mar 10 '15 at 17:15
  • \$\begingroup\$ Yeah, I see your problem. Check if the update will help. In case it won't there is one more way, but you'll have to measure performance to check if it's reasonable to use it. \$\endgroup\$ – aush Mar 10 '15 at 17:36
  • \$\begingroup\$ It looks like your update breaks the relationship between identifier type and value. The validation is attempting to make sure the combination of type and value are unique. \$\endgroup\$ – NotMyself Mar 10 '15 at 17:57
  • \$\begingroup\$ @NotMyself why do you think so? identifiersIds and identifiersValues are (kind of) "parallel" and later we check that indexes are same. \$\endgroup\$ – aush Mar 10 '15 at 18:01
  • \$\begingroup\$ ahh yeah I see that now. Let me give it a try and ill get back to you. \$\endgroup\$ – NotMyself Mar 10 '15 at 20:12
0
\$\begingroup\$

After some discussion below with @aush, here is my updated code. I have tried using IndexOf, Tuples and a struct, which were all good learning exercises. But sadly they all throw the NotSupportedException.

This code reduces the problem to a single query, but I feel it is hacky at best. I think NHibernate would have handled this issue way better than Entity Framework 6.

private void EnsureIdentifiersAreNotInUse(AddOrEditClientCommand request)
{
    var identifiers = request.Editor.Identifiers;
    var keys = identifiers.Select(i => i.TypeId + "~" + i.Value).ToArray();

    var inuse = from i in dbContext.ClientIdentifers
        where i.Client.Id != request.Editor.ClientId
        where keys.Contains(i.ClientIdentifierType.Id + "~" + i.Value)
        select i;

    if (!inuse.Any())
        return;

    for (var i = 0; i < identifiers.Count; i++)
    {
        var identifier = identifiers[i];
        var exists = inuse.Any(e => e.ClientIdentifierType.Id == identifier.TypeId
                                        && e.Value.Equals(identifier.Value));

        if (exists)
            request.ModelState.AddModelError("Identifiers[{0}].Value".FormatWith(i),
                "System Id ({0}) is already in use".FormatWith(identifier.Value));
    }
}

Also, @Dan Pantry asks why I want to avoid sprocs to solve this problem. I would rather limit the number of sprocs in the system to ones that are actually needed.

I have gone into too many codebases that have hundreds if not thousands of sprocs that nobody knows what they do or why they are even there. The sheer volume of sprocs becomes baggage that is carried by the team that can become a black box of mystery that everyone is afraid to touch.

So as an experiment on this project, I am attempting to lean on EF for all data access. Then I want to profile the application performance and add sprocs for obvious hot spots in the code.

This question is more about how can I leverage EF/linq to get the same effect without resorting to a sproc (yet..).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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