6
\$\begingroup\$

I'm using this method and I would likely have a code review. Also, I have one concern and is that source.Count() is executing synchronously and I don't know how to make it async in this context.

public static class IQueryableExtensions
    {
        public static Task<PaginatedList<T>> ToPaginatedListAsync<T>(this IQueryable<T> source, int pageSize, int pageIndex = 1)
        {
            var count = source.Count();
            return
                source.Skip((pageIndex - 1) * pageSize).Take(pageSize)
                .AsAsyncEnumerable()
                .Aggregate(new PaginatedList<T>(
                    totalCount: count,
                    pageSize : pageSize, 
                    pageIndex: pageIndex), (list, x) => { list.Add(x); return list; });
        }

    }

    public class PaginatedList<T> : List<T>
    {
        private int pageIndex;
        private int pageSize;
        public PaginatedList(IEnumerable<T> collection, int totalCount, int pageSize, int pageIndex) : base(collection)
        {
            TotalCount = totalCount;
            PageSize = pageSize;
            PageIndex = pageIndex;
        }

        public PaginatedList(int totalCount, int pageSize, int pageIndex) : base()
        {
            TotalCount = totalCount;
            PageSize = pageSize;
            PageIndex = pageIndex;
        }

        public PaginatedList(int capacity, int totalCount, int pageSize, int pageIndex) : base(capacity)
        {
            TotalCount = totalCount;
            PageSize = pageSize;
            PageIndex = pageIndex;
        }

        public int PageIndex
        {
            get { return pageIndex; }
            private set
            {
                if (value <= 0) throw new ArgumentOutOfRangeException(nameof(PageIndex));
                pageIndex = value;
            }
        }

        public int PageSize
        {
            get { return pageSize; }
            private set
            {
                if (value <= 0) throw new ArgumentOutOfRangeException(nameof(PageSize));
                pageSize = value;
            }
        }

        public int TotalPages => (int)Math.Ceiling(TotalCount / (double)PageSize);

        public bool HasPreviousPage => PageIndex > 1;

        public bool HasNextPage => PageIndex < TotalPages;

        public int TotalCount { get; }
    }

This code and updates are available at: https://gist.github.com/Bartmax/5a55ff88413174d49301

\$\endgroup\$
4
\$\begingroup\$

I have one concern and is that source.Count() is executing synchronously and I don't know how to make it async

Use CountAsync instead:

public static async Task<PaginatedList<T>> ToPaginatedListAsync<T>(this IQueryable<T> source, int pageSize, int pageIndex = 1)
{
    var count = await source.CountAsync();
    return await source.Skip() ...
}

Note that it's recommended to use ConfigureAwait(false) in "library" methods such as this one.

\$\endgroup\$
  • \$\begingroup\$ this clearly fixes my concern. and the ConfigureAwait(false) is a great addition. Thanks! \$\endgroup\$ – Bart Calixto Feb 12 '16 at 17:38
4
\$\begingroup\$
public static Task<PaginatedList<T>> ToPaginatedListAsync<T>(
  this IQueryable<T> source, int pageSize, int pageIndex = 1)

Your method says Async, returns a Task<PaginatedList<T>>, but... it doesn't say async.

public static async Task<PaginatedList<T>> ToPaginatedListAsync<T>(
  this IQueryable<T> source, int pageSize, int pageIndex = 1)

With the async keyword specified, you can now await your result - not sure what AsAsyncEnumerable does, but it doesn't seem necessary then:

return await Task.Run(() =>
{
    var count = source.Count();
    return
        source.Skip((pageIndex - 1) * pageSize).Take(pageSize)
        .Aggregate(new PaginatedList<T>(
            totalCount: count,
            pageSize: pageSize,
            pageIndex: pageIndex), (list, x) => { list.Add(x); return list; });
});

On the other hand, you're not really aggregating with Aggregate; LINQ stands for Language-INtegrated Query, and a query that has side effects isn't really a query.

source.Skip((pageIndex - 1) * pageSize).Take(pageSize)

That part is fine. Or is it? Why do you need to -1? Is pageIndex 1-based? That's pretty confusing, given the list itself is 0-based. What's the meaning of "page 0" then? Why make it throw when assigned to 0, in the setter, when the only caller for that setter is in the constructor? Better fail early and put a guard clause in the constructor and throw that ArgumentOutOfRangeException with a shorter stack trace.

I don't know why, the more I look at the Aggregate call, the more I want the method to be returning an IEnumerable<T>.

The caller already knows the totalCount (well it can have it), the pageIndex and the pageSize, and you're only ever returning a single page - I don't see why a PaginatedList<T> needs to know this metadata at that point.

If you can return an IEnumerable<T>, then you could yield the results and have this:

public static IEnumerable<T> OfPage<T>(this IQueryable<T> source, int pageIndex, int pageSize = 10)
{
    foreach (var item in source.Skip((pageIndex - 1) * pageSize).Take(pageSize))
    {
        yield return item;
    }
}

Notice I made the pageIndex mandatory, and pageSize optional - anyone using this API would expect to be able to pass a page index as a first argument to a method called OfPage.

Now, yield is pretty much the opposite of async, and it's not really buying anything here, since Take should already materialize the query, so you could have this async one-liner then:

public static async Task<IEnumerable<T>> OfPageAsync<T>(this IQueryable<T> source, int pageIndex, int pageSize = 10)
{
    return await Task.Run(() => source.Skip((pageIndex - 1)*pageSize).Take(pageSize));
}

If PaginatedList<T> doesn't need to know the totalCount, pageIndex and pageSize, then it doesn't need to exist - it's not adding any functionality, merely carrying metadata that the caller already knows, and if it needs to be carried along with the items on that page, then PaginatedList<T> can exist, but then as a composition:

public class PageItems<T> // not a list
{
    private readonly IEnumerable<T> _items;
    private readonly int _page;
    private readonly int _pageSize;

    public PageItems(IEnumerable<T> items, int page, int pageSize)
    {
        _items = items;
        _page = page;
        _pageSize = pageSize;
    }

    public IEnumerable<T> Items { get { return _items; } }
    public int PageIndex { get { return _pageIndex; } }
    public int PageSize { get { return _pageSize; } }
}

Notice that's exposing IEnumerable<T> and not List<T> or even IList<T>: if you don't need to do anything other than iterate these items, you don't need a List, you need an IEnumerable. The code doesn't change much:

public static async Task<PageItems<T>> OfPageAsync<T>(this IQueryable<T> source, int pageIndex, int pageSize = 10)
{
    return await Task.Run(() => 
    {
        var items = source.Skip((pageIndex - 1)*pageSize).Take(pageSize)
        return new PageItems(items, pageIndex, pageSize);
    });
}
\$\endgroup\$
  • \$\begingroup\$ Very good points indeed!! I used the throw in the setter to avoid having all those on 3 constructors. removing the list will fix that. I'm not aware if Task.Run instead of using await with CountAsync and ToListAsync have any side effects? Are you sure Take does materialize the query? I wonder why there is no TakeAsync method. \$\endgroup\$ – Bart Calixto Feb 12 '16 at 17:46
  • \$\begingroup\$ Why do you recommend composition instead of implementing IEnumerable interface? I like consumers to be able to foreach \$\endgroup\$ – Bart Calixto Feb 12 '16 at 18:20
  • \$\begingroup\$ It doesn't prevent foreach over the exposed IEnumerable<Item>, and composition doesn't prevent implementing IEnumerable and an indexed getter and whatnot... I don't see any problem with composition. \$\endgroup\$ – Mathieu Guindon Feb 12 '16 at 18:25
  • \$\begingroup\$ Thank you Mat, I included a link for code and revisions in the question. Let me know if it looks good! Updates were made based on the great feedback from all answers/comments. \$\endgroup\$ – Bart Calixto Feb 12 '16 at 19:14
  • 1
    \$\begingroup\$ @BartCalixto " I'm not aware if Task.Run instead of using await with CountAsync and ToListAsync have any side effects?" -- I'd recommend Stephen Cleary's blog post Don't Use Task.Run in the Implementation. \$\endgroup\$ – mjolka Feb 12 '16 at 23:02
2
\$\begingroup\$

This is a bit odd:

return
    source.Skip((pageIndex - 1) * pageSize).Take(pageSize)
    .AsAsyncEnumerable()
    .Aggregate(new PaginatedList<T>(
        totalCount: count,
        pageSize : pageSize, 
        pageIndex: pageIndex), (list, x) => { list.Add(x); return list; });
    }

It seems like all you really want is:

var pagedList = new PaginatedList<T>(count, pageSize, pageIndex);
var pageItems = source.Skip((pageIndex - 1) * pageSize).Take(pageSize);
pagedList.AddRange(pageItems);
return pagedList;

I've removed your labels for the constructor parameters as I think it's fairly obvious without them. Cleaning it all up and including advice from other answers:

public static class IQueryableExtensions
{
    public static async Task<PaginatedList<T>> ToPaginatedListAsync<T>(this IQueryable<T> source, int pageSize, int pageIndex = 1)
    {
        if (source == null) 
        {
            throw new ArgumentNullException("source");
        }

        var totalCount = await source.CountAsync().ConfigureAwait(false);
        var pagedList = new PaginatedList<T>(totalCount, pageSize, pageIndex);
        var pageItems = source.Skip((pageIndex - 1) * pageSize).Take(pageSize);
        pagedList.AddRange(pageItems);
        return pagedList;
    }
}

You could make getting the items async too by doing:

 var pageItems = await source.Skip((pageIndex - 1) * pageSize).Take(pageSize).ToListAsync().ConfigureAwait(false);
 pagedList.AddRange(pageItems);
\$\endgroup\$
  • \$\begingroup\$ i used the labels because there's another constructor that accepts same type arguments (int,int,int) and may be prone to errors if changed. I'm wondering if do I really want/need a list and all those ctros... \$\endgroup\$ – Bart Calixto Feb 12 '16 at 17:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.