19

I currently have this LINQ/EF code in my application:

var rootCategoryItem = DatabaseContext.Categories
                            .Include("SubCategories")
                            .OrderBy(c => c.CategoryOrder)
                            .Single(c => c.CategoryId == 1);

I know in EF you can't filter Included items yet, and I can write some LINQ code to filter out the SubCategories that aren't needed... but the LINQ code gets converted to a horrendous SQL which is highly un-optimised. I could also write a stored proc that does this (and write a much better query than LINQ), but I really want to use pure EF.

So I'm left with 2 options (unless someone can see other options).

The first is to loop through the subcategories, remove the ones that aren't needed:

        var subCategoriesToFilter = rootCategoryItem.SubCategories.ToList();

        for (int i = 0; i < subCategoriesToFilter.Count; i++)
        {
            if (subCategoriesToFilter[i].Deleted)
                rootCategoryItem.SubCategories.Remove(subCategoriesToFilter[i]);
        }

The second option would be to have this in my view:

<ul class="treeview ui-accordion-content ui-helper-reset ui-widget-content ui-corner-bottom ui-accordion ui-widget ui-sortable ui-accordion-content-active">
@foreach (var categoryitem in Model.SubCategories.OrderBy(c => c.CategoryOrder))
{

    @if(!Model.Deleted)
    { 
        <li class="treelistitem" id="@Model.CategoryId">
            <div class="ui-accordion-header ui-state-default ui-corner-all ui-accordion-icons ui-sortable-handle first">
            <span class="clickable">
                <span class="ui-accordion-header-icon ui-icon treeviewicon treeviewplus"></span>
                <i class="glyphicon glyphicon-folder-open rightfolderpadding"></i><span class="categoryname">@Model.CategoryName</span>
            </span>
            </div>
           </li>
    }
}   
</ul>

Out of the 2, which one would be the best option? Or is there another option I'm missing?

The Solution

OK, Servy's is pretty much correct, I had to modify his answer to make it work:

        var rootCategoryItem = DatabaseContext.Categories
            .OrderBy(c => c.CategoryId)
            .ToList().Select(c => new Category()
            {
                SubCategories = c.SubCategories.Where(sub => !sub.Deleted).ToList(),    //make sure only undeleted subcategories are returned
                CategoryId = c.CategoryId,
                CategoryName = c.CategoryName,
                Category_ParentID = c.Category_ParentID,
                CategoryOrder = c.CategoryOrder,
                Parent_Category = c.Parent_Category,
                Deleted = c.Deleted
            }).Single(c => c.CategoryId == 1);

I had several errors trying to get Servy's solution to work:

The entity or complex type '.Category' cannot be constructed in a LINQ to Entities query

Cannot implicitly convert type to System.Collections.Generic.ICollection. An explicit conversion exists (are you missing a cast?)

This was all resolved by adding .ToList() before the Select() method.

binks
  • 1,001
  • 2
  • 10
  • 26
  • 8
    Are you sure your solution is correct? That ToList() that you added will load the entire Categories table from the database. – Josh Mouch Jan 21 '16 at 23:12
  • 1
    exactly what @JoshMouch said. it will work because you're returning the whole table, and then using LinqToEntities on the result to filter it further. I am trying to save the database load as well :) will report if i find a better way – Sonic Soul Apr 04 '17 at 23:17

3 Answers3

24

While you cannot filter a collection included via Include, you can use Select and project that collection into a filtered collection.

var rootCategoryItem = DatabaseContext.Categories
    .OrderBy(c => c.CategoryOrder)
    .Select(c => new Category()
    {
        SubCategories = c.SubCategories.Where(sub => !sub.Deleted)
            .OrderBy(sub => sub.CategoryOrder),
        c.CategoryId,
        c.CategoryName,
        //include any other fields needed here
    })
    .Single(c => c.CategoryId == 1);
Servy
  • 202,030
  • 26
  • 332
  • 449
  • 2
    I do like this solution, however, I get an error on this line: SubCategories = c.SubCategories.Where(sub => !sub.Deleted) .OrderBy(sub => sub.CategoryOrder), Cannot implicitly convert type 'System.Linq.IOrderedEnumerable' to 'System.Collections.Generic.ICollection'. An explicit conversion exists (are you missing a cast?) – binks Sep 02 '14 at 18:05
  • @binks Then materialize the `IEnumerable` to an `ICollection`. – Servy Sep 02 '14 at 18:08
  • 1
    Hm... I've tried casting (ICollection)c.SubCategories.Where(sub => !sub.Deleted) .OrderBy(sub => sub.CategoryOrder), Although that works, I then get a new error: The entity or complex type '.Category' cannot be constructed in a LINQ to Entities query. – binks Sep 02 '14 at 18:13
  • @binks You can't cast it. You need to convert it. You need to use a method that can take any given sequence and create a collection based on that sequence. Telling the compiler to pretend that it's a collection when it's not won't help. – Servy Sep 02 '14 at 18:15
  • 2
    OK, but using .ToList() should work as it inherits from ICollection... but I still get the new error: The entity or complex type '.Category' cannot be constructed in a LINQ to Entities query. – binks Sep 02 '14 at 18:23
  • 1
    @binks Then you'll need to not use the `Category` type in the projection. Project out to an anonymous type, or a new type. – Servy Sep 02 '14 at 18:24
  • This solution works but still loads all objects from the database. It is up to you if the performance hit is acceptable or not. – Vladimir Jul 22 '15 at 08:04
  • @uraster No, it does not load all of the objects from the database. It loads only those in the final result, and not any of the intermediate results. – Servy Jul 22 '15 at 13:00
  • @Servy Yes, you are probably right. How many calls are made to the database (I am not able to test it right now)? I see it so: one call when the ToList() is reached (the final solution of the OP) and then one call for each Category to load the SubCategories. If my assumption is correct, the number of calls might be substantial and then loading everything and filtering on the C# side might be more efficient. – Vladimir Jul 22 '15 at 16:33
  • @uraster The OP put a `ToList` in his solution incorrectly. In his solution he's fetching information he doesn't need from the database. If he simply doesn't do that then everything is fetched in a single query that only gets exactly the information he needs.. Your critique only applies to the OP's answer, not mine. – Servy Jul 22 '15 at 16:51
  • @Servy No critique at all, sorry that it sounded like that. I just want to clarify. I now tested both solutions: the OP's solution causes one call to the db to get all categories and then a separate call to the db for each category to get its subcategories. The filtering for deleted happens on the C# side and not on the db side. Your solution doesn't compile without a ToList() in the SubCategories = c.SubCategories.Where(...) line and when I add ToList there is an exception: The entity or complex type 'TestModel.Category' cannot be constructed in a LINQ to Entities query. – Vladimir Jul 22 '15 at 20:23
  • @uraster As was mentioned in an earlier comment, you can simply use an anonymous type instead of the entity type in the `select` to solve that problem. – Servy Jul 23 '15 at 13:26
4

I recon this way looks more cleaner and shorter. Not sure about database impact

 var rootCategoryItem = DatabaseContext.Categories.SingleOrDefault();
 if (rootCategoryItem == null) return null;
 {
   rootCategoryItem.Items = rootCategoryItem ?.Items.Where(x => !x.IsDeleted).ToList();
   return rootCategoryItem;
  }
CodeNotFound
  • 22,153
  • 10
  • 68
  • 69
Aleksei
  • 1,107
  • 9
  • 13
  • if you had a ton of rootCategoryItem.Item in your db, you'd be bringing them and your db would be mapping them for no reason. I realize this is 6 years ago, but thought i'd mention it just in case. – Dylan Hayes Jan 06 '23 at 03:08
0

Your concerns here are a presentation concern (only showing non-deleted categories). That would indicate Method 2 as your best choice.

HOWEVER, I suspect that you need to use non-deleted categories often in your system. This would indicate that you should have a function that returns non-deleted categories consistently for your use anywhere that you need it.

For that reason, I would recommend method 1.

Chuck Buford
  • 321
  • 1
  • 9