0
\$\begingroup\$

I have written the following Query to retrieve some object's ID. If there are objects in the list that have a larger numbinbatch than the saveditems number in the batch, then it should take that. If not, then it should take the first number from the lower number in batches.

I am wondering if this could be shortened. I'd also like to know if my query is bad for performance and how it can be improved.

int NextItemID = 0;
var items =
    rep.FindWhere(i => i.ID == itemID)
    .Select(i => i.Batch)
    .SelectMany(b => b.Items);

    var savedItem = items.First(i => i.ID == itemID);
    var unsavedItems = items.Where(i => i.StatusID == (short) ItemStatus.Pending);

    var higherNumInBatch = unsavedItems.Where(i => i.NumInBatch > savedItem.NumInBatch).OrderBy(i=>i.NumInBatch);
    var lowerNumInBatch = unsavedItems.Where(i => i.NumInBatch < savedItem.NumInBatch).OrderBy(i=>i.NumInBatch);

    if (higherNumInBatch.Any())
    {
       NextItemID = (int)higherNumInBatch.Take(1).First().ID;
    }
    else
    {
       NextItemID = (int)lowerNumInBatch.Take(1).First().ID;
    }

    return NextItemID;
\$\endgroup\$

3 Answers 3

2
\$\begingroup\$

This will be shorter, it takes less selects, and may runs at SQL server side

return rep.FindWhere(i => i.ID == itemID)
          .SelectMany(i => i.Batch.Items)
          .Where(i => i.StatusID == (short) ItemStatus.Pending)
          .MaxBy(i => i.NumInBatch).ID;
\$\endgroup\$
1
\$\begingroup\$

You should use MaxBy() from here, which is taken from MoreLINQ.

var items = rep.FindWhere(i => i.ID == itemID)
               .Select(i => i.Batch)
               .SelectMany(b => b.Items);

var NextItem = items.Where(i => i.StatusID == (short) ItemStatus.Pending)
                    .MaxBy(i => i.NumInBatch);

return NextItem.ID;
\$\endgroup\$
0
\$\begingroup\$

What you certainly can/should do is this:

var higherNumInBatch = unsavedItems.Where(i => i.NumInBatch > savedItem.NumInBatch).OrderBy(i=>i.NumInBatch);

if (higherNumInBatch.Any())
{
    NextItemID = (int)higherNumInBatch.Take(1).First().ID;
}
else
{
    var lowerNumInBatch = unsavedItems.Where(i => i.NumInBatch < savedItem.NumInBatch).OrderBy(i=>i.NumInBatch);
    NextItemID = (int)lowerNumInBatch.Take(1).First().ID;
}

This way you'll only fetch the lower numbers in the batch only if there is no higher one. In your case you're always getting both lists which is not always necessary.

\$\endgroup\$
2
  • \$\begingroup\$ What if unsavedItems is empty. Take(1).First() will fail. Also using .Any() on highNumberInBatch and then .Take() potentially enumerates the list twice. Consider using ToList() first. \$\endgroup\$
    – dreza
    Commented Jul 11, 2013 at 0:26
  • \$\begingroup\$ You're right, there should be an extry check to see if 'unsavedItems' has any items at all. :) \$\endgroup\$
    – Abbas
    Commented Jul 11, 2013 at 7:07

Your Answer

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

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