1
\$\begingroup\$

Any advice on how to make this code; cleaner, more effective, just overall better!

Program has three options that can be passed int NewsId,Year and CategoryId. I collection all the data and then do additional filter base on the values that are being passed into the method.

public List<News> GetNewsbyYear(int NewsId, int Year, int CategoryId)
{
 using (var db = new NccnEcommerceEntities())
 {
 var listOfNews = (from n in db.News
 where n.Display == true
 select new Model.News
 {
 NewsId = n.NewsId,
 Title = n.Title,
 ReleaseDate = n.ReleaseDate,
 Body = n.Body,
 ExternalUrl = n.ExternalUrl,
 LastUpdated = n.LastUpdated,
 DisplayInBlog = n.DisplayInBlog,
 BoilerPlate = n.BoilerPlate,
 ItemDictionary = (from x in db.NewsCategoryXrefs
 join i in db.DboItemDictionaries on x.CategoryId equals i.ItemDictionaryId
 select new Model.ItemDictionary
 {
 Name = i.Name,
 ItemDictionaryId = i.ItemDictionaryId
 }).FirstOrDefault()
 });
 if (Year > 0)
 {
 listOfNews = listOfNews.Where(item => item.ReleaseDate.Value.Year == Year);
 }
 if (CategoryId > 0)
 {
 listOfNews = listOfNews.Where(item => item.ItemDictionary.ItemDictionaryId == CategoryId);
 }
 if (NewsId > 0)
 {
 listOfNews = listOfNews.Where(item => item.NewsId == NewsId);
 }
 return listOfNews.ToList();
 }
}
asked Jul 13, 2020 at 17:12
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

It looks to me that you can filter the data in the original query, instead compiling the query then filtering it.

It's difficult to suggest an exact solution without access to the required objects and collections, but something like this can give you a start:

public List<News> GetNewsbyYear(int NewsId, int Year, int CategoryId)
{
 using (var db = new NccnEcommerceEntities())
 {
 return (from n in db.News
 where n.Display == true && 
 ((Year > 0 && n.ReleaseDate.Value.Year == Year) ||
 (NewsId > 0 && n.NewsId == NewsId) ||
 (CategorId > 0)) 
 select new Model.News
 {
 NewsId = n.NewsId,
 Title = n.Title,
 ReleaseDate = n.ReleaseDate,
 Body = n.Body,
 ExternalUrl = n.ExternalUrl,
 LastUpdated = n.LastUpdated,
 DisplayInBlog = n.DisplayInBlog,
 BoilerPlate = n.BoilerPlate,
 ItemDictionary = (from x in db.NewsCategoryXrefs
 where CategoryID > 0 && x.CategoryId == CategoryId
 join i in db.DboItemDictionaries on x.CategoryId equals i.ItemDictionaryId
 select new Model.ItemDictionary
 {
 Name = i.Name,
 ItemDictionaryId = i.ItemDictionaryId
 }).FirstOrDefault()
 }).toList();
 }
}
answered Jul 13, 2020 at 18:00
\$\endgroup\$
3
  • \$\begingroup\$ Is this way better then using the filter? why would I use this vs the other way? \$\endgroup\$ Commented Jul 13, 2020 at 18:03
  • \$\begingroup\$ If I'm not mistaken, calling Where on an existing query will compile it first, resulting in the query being compiled, filter called then compiled again to a list. \$\endgroup\$ Commented Jul 13, 2020 at 18:07
  • \$\begingroup\$ Unfortunately, you are mistaken. At least, if this is Entity Framework or any other popular ORM. The entire query is translated into SQL. The code in the question is usually much better, because in your alternative, by most ORMs, all conditions, including Year > 0are translated into SQL, giving the query optimizer more trouble than necessary, which may actually lead to an inferior query plan. AFAIK, only EF core 3 is capable of removing redundant predicates from the query while translating it. \$\endgroup\$ Commented Jul 25, 2020 at 14:19

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.