pagination in entity framework and refactoring

Question

I have an api end point which returns the paginated result, the corresponding code for that is as below:

public Task<PaginatedDataResult<ProductSearchResult>> GetPaginatedSearchAsync(int currentPage,
                                                                                      int perPage,
                                                                                      string nameFilter,
                                                                                      string sortBy,
                                                                                      bool sortDesc,
                                                                                      CancellationToken cancellationToken = default)
        {
            IEnumerable<Expression<Func<Product, bool>>> GetFilters()
            {
                if (!string.IsNullOrEmpty(nameFilter))
                {
                    var normalizedNameFilter = nameFilter.NormalizeUpper();
                    yield return p => p.NormalizedName.Contains(normalizedNameFilter);
                }
            }

        Expression<Func<Product, ProductSearchResult>> projectionExpression =
            p => new ProductSearchResult
            {
                Id = p.Id,
                Name = p.Name,
                EndDate = p.EndDate,
                IsActive = p.IsActive,
                ServicesCount = p.ProductServices.Count
            };

        return PaginatedFilterWithProjectionAsync(currentPage, perPage, sortBy, sortDesc, cancellationToken, projectionExpression, GetFilters().ToArray());
    }
    
    
    protected async Task<PaginatedDataResult<TProjectedEntity>>
        PaginatedFilterWithProjectionAsync<TProjectedEntity>(int currentPage,
            int perPage,
            string sortBy,
            bool sortDesc,
            CancellationToken cancellationToken,
            Expression<Func<TEntity, TProjectedEntity>> projectionExpression,
            params Expression<Func<TEntity, bool>>[] filters
        ) where TProjectedEntity : class
    {
        var query = filters.Aggregate(DbSet.AsQueryable(), (q, f) => q.Where(f));
        var queryWithProjection = query.Select(projectionExpression);


        if (!string.IsNullOrEmpty(sortBy))
        {
            queryWithProjection = sortDesc
                ? queryWithProjection.OrderByDescending(sortBy)
                : queryWithProjection.OrderBy(sortBy);
        }

        var filteredQueryOfProjectedEntity = queryWithProjection
            .Skip((currentPage - 1) * perPage)
            .Take(perPage);

        var filteredListOfProjectedEntity = await filteredQueryOfProjectedEntity.ToListAsync(cancellationToken);

        var count = await queryWithProjection.CountAsync(cancellationToken);

        return new PaginatedDataResult<TProjectedEntity>
        {
            PerPage = perPage,
            CurrentPage = currentPage,
            TotalCount = count,
            Data = filteredListOfProjectedEntity
        };
    }

Now, I have another end point which is returning a complete collection as below

    public async Task<ICollection<ProductRelationship>> GetAllByRelationshipDataAsync(
        bool includeDetail,
        bool filterActiveProduct = false,
        DateTime? filterActiveRelationshipDate = null,
        CancellationToken cancellationToken = default)
    {            

        var set = includeDetail ? IncludeDetail(DbSet) : DbSet;
       
        //I have removed few parameters from the method and also few if conditions for brevity
        if (filterActiveProduct)
        {
            set = set.Where(x => x.Product != null && x.Product.IsActive);
        }

        if (filterActiveRelationshipDate.HasValue)
        {
            var filterDate = filterActiveRelationshipDate.Value.Date;
            set = set.Where(x => x.StartDate <= filterDate && (!x.EndDate.HasValue || x.EndDate.Value >= filterDate));
        }

        return await set.ToListAsync(cancellationToken);
    }

The requirement is to change the above method to return a paginated result, so the changes I did are as below:

change the method signature to add four additional parameters (currentPage, perPage, sortBy, sortDesc)

Rather than returning a collection, calling a PaginatedAsync method to return paginated result

         public async Task<PaginatedDataResult<ProductRelationship>> GetAllByRelationshipDataAsyncTest(
 bool includeDetail,       
 int currentPage,
 int perPage,
 string sortBy,
 bool sortDesc,
 bool filterActiveProduct = false,
 DateTime? filterActiveRelationshipDate = null,
 CancellationToken cancellationToken = default)
     {                

         var set = includeDetail ? IncludeDetail(DbSet) : DbSet;

         //I have removed few parameters from the method and also few if conditions for brevity

         if (filterActiveProduct)
         {
             set = set.Where(x => x.Product != null && x.Product.IsActive);
         }

         if (filterActiveRelationshipDate.HasValue)
         {
             var filterDate = filterActiveRelationshipDate.Value.Date;
             set = set.Where(x => x.StartDate <= filterDate && (!x.EndDate.HasValue || x.EndDate.Value >= filterDate));
         }

         return await PaginatedAsync(set, currentPage, perPage, sortBy, sortDesc, cancellationToken);
     }

PaginatedAsync method is the same copy of PaginatedFilterWithProjectionAsync method above but without the variables query and queryWithProjection, since I have got the queryWithProjection value in the variable set above.
I could also refactor the PaginatedFilterWithProjectionAsync method to include PaginatedAsync as most of the code is same, but that was already in use, that adds more testing effort. May be I am breaking open closed principle

protected async Task<PaginatedDataResult<TProjectedEntity>> PaginatedAsync<TProjectedEntity>(IQueryable<TProjectedEntity> queryWithProjection, int currentPage,
    int perPage,
    string sortBy,
    bool sortDesc,
    CancellationToken cancellationToken)
{
    if (!string.IsNullOrEmpty(sortBy))
    {
        queryWithProjection = sortDesc
            ? queryWithProjection.OrderByDescending(sortBy)
            : queryWithProjection.OrderBy(sortBy);
    }

    var filteredQueryOfProjectedEntity = queryWithProjection
        .Skip((currentPage - 1) * perPage)
        .Take(perPage);

    var filteredListOfProjectedEntity = await filteredQueryOfProjectedEntity.ToListAsync(cancellationToken);

    var count = await queryWithProjection.CountAsync(cancellationToken);

    return new PaginatedDataResult<TProjectedEntity>
    {
        PerPage = perPage,
        CurrentPage = currentPage,
        TotalCount = count,
        Data = filteredListOfProjectedEntity
    };
}

Please can anyone review and provide me with suggestions for better approach and point me the mistakes in the above code.

BCdotWEB · Accepted Answer · 2020-06-19 23:00:35Z

Some quick remarks:

There are almost 90 white-space characters before I can see the arguments to GetPaginatedSearchAsync. In such cases, move the parameters to the next line.
Consider renaming PaginatedDataResult and ProductSearchResult (is the "Result" part of the name necessary?), and even GetPaginatedSearchAsync (is there a non-async method? if not, then why add this to the method name?).
GetPaginatedSearchAsync has six parameters: that is bordering on the limit, and I'd advise you to pass a custom class instead. You could at least group the pagination-related parameters into their own class.
Do not pointlessly abbreviate names. I get what sortDesc means but it still is jarring to read.
Judging by //I have removed few parameters from the method and also few if conditions for brevity it seems you realized the amount of parameters grew too large and impacted readability. Consider moving all of the filter-related parameters into a class of their own and pass that class instead of an ever-growing and thus hard to maintain list.
IMHO you should avoid pointlessly detailed variable names like filteredQueryOfProjectedEntity and filteredListOfProjectedEntity, and even queryWithProjection (why assign query only to immediately apply Select to it on the next line and then never use it again? why not combine these two lines and simply use query instead of queryWithProjection?). These make your code hard to read. Instead, I'd expect these names to convey more of a concept.

Gert Arnold · Accepted Answer · 2020-07-01 10:07:57Z

One main remark: the paging method is too busy. The number of arguments is a tell-tale. The parts that don't belong there are filtering and sorting.

In general, a method returning any paged result, should only receive an IQueryable and the paging instructions page number and page size. Your second method is almost there. After removing the superfluous responsibilities, what's left is:

protected async Task<PaginatedDataResult<TEntity>> PaginateAsync<TEntity>(
    IQueryable<TEntity> query, int pageNumber, int pageSize,
    CancellationToken cancellationToken)
{
    var pageQuery = query
        .Skip((pageNumber - 1) * pageSize)
        .Take(pageSize);

    var page = await pageQuery.ToListAsync(cancellationToken);

    var count = await query.CountAsync(cancellationToken);

    return new PaginatedDataResult<TEntity>
    {
        PerPage = pageSize,
        CurrentPage = pageNumber,
        TotalCount = count,
        Data = page
    };
}

Now you have a method that you can offer any IQueryable. It doesn't need to know if it's a DbSet or a projection, or filtered, or ordered. The count of the query is independent of all these details. [In fact, when using Entity Framework, you won't even see the projection in the generated SQL count query)].

This makes each method that calls PaginateAsync responsible for composing the IQueryable they want to have paged. And —the main point— now it's possible to have a method that just returns the IQueryable and another method that takes this IQueryable and returns a page.

So, you'll just keep GetAllByRelationshipDataAsync, which I would at least shorten to GetAllByRelationshipAsync, and add a method GetAllByRelationshipPagedAsync that looks like your GetAllByRelationshipDataAsyncTest method, but now internally just executes GetAllByRelationshipAsync and PaginateAsync.

Likewise, GetPaginatedSearchAsync should be changed to contain the filtering and ordering parts, so it will be able to call PaginateAsync as well.

Of course, if you want, you can create separate methods that execute filtering and/or ordering.

Now it's even possible to have a service layer that's just bothered with composing queries and doesn't even contain any paging logic. The paging method could be part of the controller layer only.

Stack Exchange Network

pagination in entity framework and refactoring

2 Answers 2

You must log in to answer this question.

Hot Network Questions

pagination in entity framework and refactoring

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions