Query results with many to many and some parameters using Entity Framework in a more efficiënt way

I've created a working query in Entity Framework and I'm curious if there's a better way to do this. Since there's a lot of field on my models, I will only publish the relevant ones here.

My Models are like this:

public class Category
{
public int Id { get; set; }

private ICollection products = new List();
public virtual ICollection Products { get { return products; } set { products = value; } }

private ICollection languages = new List();
public virtual ICollection Languages { get { return languages; } set { languages = value; } }
}

public class CategoryLanguage
{
public virtual int Id { get; set; }
public virtual Language Language { get; set; }
public virtual string Slug { get; set; }
public virtual string Title { get; set; }
public virtual string Description { get; set; }
public virtual string ShortDescription { get; set; }
}

public class Language
{
public virtual int Id { get; set; }
public virtual string Iso { get; set; }
}

public class Product
{
public int Id { get; set; }

private ICollection languages = new List();
public virtual ICollection Languages { get { return languages; } set { languages = value; } }
}

public class ProductLanguage
{
public virtual int Id { get; set; }
public virtual Language Language { get; set; }
public virtual string Title { get; set; }
public virtual string Description { get; set; }
public virtual string ShortDescription { get; set; }
public virtual string Slug { get; set; }
}


My Result models looks like this:

public class ShowProductsModel
{
public string Category { get; set; }

public IEnumerable Products { get; set; }
}

public class ShowProductsProductModel
{
public int Id { get; set; }

public string Title { get; set; }

public string ShortDescription { get; set; }
}


My Entity Framework query looks like this:

`public override async Task Execute()
{
string culture = "someCulture";
string categoryN

private ICollection products = new List();
public virtual ICollection Products { get { return products; } set { products = value; } }

You don't have to get rid of your automatically-implemented properties to make sure your collections are initialized. Instead, use the default constructor to do this.

public Category()
{
    Products = new List();
}

public virtual ICollection Products { get; set; }

public override async Task Execute()

According to convention, asynchronous methods should have the "Async" postfix to indicate this.

This turns into

public override async Task ExecuteAsync()

You indicate that only one category should be queried. This contrasts your declaration of

.Where(c => c.Languages.Any(l => l.Slug == categoryName && l.Language.Iso == culture))

Instead I would use SingleOrDefault():

.SingleOrDefault(c => c.Languages.Any(l => l.Slug == categoryName && l.Language.Iso == culture))

Products = cat.Products
              .Where(p => p.Categories.Any(c => 
                             c.Languages.Any(l => 
                                 l.Slug == categoryName && l.Language.Iso == culture)))

This query was already executed before. In fact, this is the query you started with except now you suddenly start from the viewpoint of Products instead of Categories. You still have access to cat in this context which is the category that you're working with.

What you do here is loop over all those products in that category, look at all their categories and then take the first one that has your specified information, essentially doing work that isn't needed.

I can't execute it to verify of course but I believe you can simply omit this entire Where clause unless I'm missing something.

There is a distinction between FirstAndDefault() and SingleAndDefault() that is relevant to other programmers reading your code: the former says there can be more than one, but take one and return default otherwise. The latter says there can only be one so take that one and return default otherwise.

I believe you can change all of these to SingleAndDefault() aside from your very last one in the return statement.

Which brings me to this: by using SingleOrDefault() at the start you no longer have a collection on which you perform Select() but a single object. This makes sure you can drop the remaining FirstOrDefaultAsync() since the Select() will already return a single object.

StackExchange Code Review Q#77809, answer score: 5