dncuug / X.PagedList

Library for easily paging through any IEnumerable/IQueryable in ASP.NET
https://andrew.gubskiy.com/open-source
MIT License
899 stars 213 forks source link

X.PagedList: ToPagedList: Strange condition #266

Closed adschmu closed 3 months ago

adschmu commented 3 months ago

In ToPagedList(superset, pageNumber, pageSize, totalSetCount), there is a strange condition which looks to me like && and || might be used incorrectly or the logic is flawed otherwise:

https://github.com/dncuug/X.PagedList/blob/master/src/X.PagedList/PagedListExtensions.cs#L218-L227

        if ((totalCount <= 0 || totalSetCount.HasValue) && supersetCount <= pageSize)
        {
            subset = superset.ToList();
        }
        else
        {
            var skip = (pageNumber - 1) * pageSize;

            subset = superset.Skip(skip).Take(pageSize).ToList();
        }

If the supersetCount is below pageSize, I would expect to always go with .ToList(), since paging makes no sense in this case. For totalCount < 0, we do not need to query at all, but may simply use an empty list instead. For totalSetCount.HasValue, we know that we can use this instead of using Count(), but this is in the wrong place.

I do not see the logic in this, and would be delighted if somebody could show it to me or confirm that this is broken otherwise.

I also do not see a point in always calling Count() even if a totalSetCount is provided (slightly before the part shown); I would rather prefer to get around the Count() (at least for a Queryable) and do the check after the query.

In total, I would expect exactly the sync version of the ToPagedListAsync() in the EF project.

Please comment.