Am I getting list in a good way in EF

Question

I am still new in entity framework. So forgive me if question is dummy :)
I have a domain class that get's a list of some data from database:

public IEnumerable<Item> GetItems()
        {
            return context.Items.ToList();
        }

This code return all items from database.
On the site I use paging so I need only 10 items per page.
So I did something like this:

var model = itemsRepository.GetItems().
                    Where(x => x.CategoryId == categoryId).
                    OrderByDescending(x => x.CreatedOnDate).
                    Skip(0).
                    Take(pageSize);

Now as I see what I did here is, I take all items from db and filter them.
Will I get some benefit if I put new method in domain and put the following code in it:

return context.Items.Where(x => x.CategoryId == categoryId).
                    OrderByDescending(x => x.CreatedOnDate).
                    Skip(0).
                    Take(pageSize);

note that you should have a variable for skip which should be something like 'int skip = pageSize * pageNumber-1' (untested) — Nathan Koop, Aug 30 '12 at 21:03

score 1 · Answer 1 · answered Aug 30 '12 at 20:55

Yes, you should. I'm assuming you're using SQL as the backend for your context, but the query that ends up getting constructed with your new method will only pull those 10 records out and return them as an IEnumerable (deferred execution) rather than pulling everything from the database and then just filtering out the first 10 results.

I think you're better off with the second (new) method using deferred execution.

Are you seeing an improvement in performance via SQL Profiler, too?

score 1 · Accepted Answer · edited May 23 '17 at 12:04

Yes. You will get the benefit that your LINQ query in the latter case will get translated to SQL and executed in the database. Therefore, your first example will load the entire table into memory - while the second example will do a much more efficient query in the database.

Essentially, the .ToList() breaks deferred execution - but it might also make sense for you to return IQueryable<T> rather than IEnumerable<T>, and then working on that in upper layers - depending on your requirements. Also, try reading this question and answer.

score 1 · Answer 3 · answered Aug 30 '12 at 21:00

There are some problems in your code:

Do not use variable in class for context, every time you need it, create it and dispose it (with using):

using(var context = new ...) { // do DB stuffs }
Do not call ToList(), to fetch all items, use normal paging then call ToList (something like your second sample but with using, ...).

score 1 · Answer 4 · answered Aug 30 '12 at 21:00

The problem with the second approach is that the domain now is coupled to the context. This defeats one of the main purposes of the repository pattern. I suggest you have the second method inside the repository where you passed it the page number you want retrieved and it returns them to you. In your repository have something like

public IEnumerable<Item> GetItemsForPage(int pageNumber)
    {
        return context.Items.Where(x => x.CategoryId == categoryId).
                OrderByDescending(x => x.CreatedOnDate).
                Skip(pageNumber * pageSize).  //Note not always 0
                Take(pageSize);

    }

In your domain you would call repository.GetItemsForPage(). This gives you the benefit of delayed execution while maintaining the decoupling of domain and context.

Am I getting list in a good way in EF

4 Answers4