patterncsharpMinor
ToPaginatedListAsync<T>
Viewed 0 times
topaginatedlistasyncstackoverflowprogramming
Problem
I'm using this method and I would likely have a code review. Also, I have one concern and is that
This code and updates are available at: https://gist.github.com/Bartmax/5a55ff88413174d49
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> ToPaginatedListAsync(this IQueryable source, int pageSize, int pageIndex = 1)
{
var count = source.Count();
return
source.Skip((pageIndex - 1) * pageSize).Take(pageSize)
.AsAsyncEnumerable()
.Aggregate(new PaginatedList(
totalCount: count,
pageSize : pageSize,
pageIndex: pageIndex), (list, x) => { list.Add(x); return list; });
}
}
public class PaginatedList : List
{
private int pageIndex;
private int pageSize;
public PaginatedList(IEnumerable 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 (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/5a55ff88413174d49
Solution
public static Task> ToPaginatedListAsync(
this IQueryable source, int pageSize, int pageIndex = 1)Your method says
Async, returns a Task>, but... it doesn't say async.public static async Task> ToPaginatedListAsync(
this IQueryable 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(
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.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 needs to know this metadata at that point.If you can return an
IEnumerable, then you could yield the results and have this:public static IEnumerable OfPage(this IQueryable 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> OfPageAsync(this IQueryable source, int pageIndex, int pageSize = 10)
{
return await Task.Run(() => source.Skip((pageIndex - 1)*pageSize).Take(pageSize));
}If
PaginatedList 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 can exist, but then as a composition:public class PageItems // not a list
{
private readonly IEnumerable _items;
private readonly int _page;
private readonly int _pageSize;
public PageItems(IEnumerable items, int page, int pageSize)
{
_items = items;
_page = page;
_pageSize = pageSize;
}
public IEnumerable Items { get { return _items; } }
public int PageIndex { get { return _pageIndex; } }
public int PageSize { get { return _pageSize; } }
}Notice that's exposing
IEnumerable and not List or even IList: 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> OfPageAsync(this IQueryable 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);
});
}Code Snippets
public static Task<PaginatedList<T>> ToPaginatedListAsync<T>(
this IQueryable<T> source, int pageSize, int pageIndex = 1)public static async Task<PaginatedList<T>> ToPaginatedListAsync<T>(
this IQueryable<T> source, int pageSize, int pageIndex = 1)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; });
});source.Skip((pageIndex - 1) * pageSize).Take(pageSize)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;
}
}Context
StackExchange Code Review Q#119721, answer score: 4
Revisions (0)
No revisions yet.