Setting up pagination links inside a WebAPI Controller

Question

I'm implementing paging for the first time and have the following code which achieves what I want.

There are just a couple of things that feel a bit weird to me and would like if somebody could give it a once over and see if it's okay.

At the moment, I call a service to retrieve objects from a DB using a repository class.

When I have all matching objects - I pass them to a "Paging" method within the service (I think I have to do this in the service rather than the repository because a generic repo paging method won't work across some resources e.g "orderBy" a property nested within a class in an aggregate object).

Once paging is complete, the result is passed back to the WebAPI Controller where I have another method to setup the page linking information (current, next, previous, first and last page links.)

This method feels a bit hacky and doesn't feel like it should be in the controller - it could probably be used across other endpoints, maybe it should go into a Util class or pull it out into the pipeline.

Controller.cs

[Route("v1/endpoint")]
[HttpGet]
//[ValidateFilter] TODO Allow optional params + validate
public async Task<IHttpActionResult> GetConnectionsAsync([FromUri]List<ConnectionStatus> connectionStatus, [FromUri]int page = 1,
    [FromUri]int pageSize = 20, [FromUri]string orderBy = "name", [FromUri]bool ascending = true)
{
        var id = await AuthorizationService.GetAzureObjectIdentifierAsync();

        var objects = await Service.GetObjectsAsync(id, connectionStatus, page, pageSize, orderBy, ascending);

        //TODO Should this be in the controller? Or Should it be pulled out into the pipeline?
        objects.PagingMetaData.PageLinkInfo = SetupPagingLinks(Url, page, pageSize, objects.PagingMetaData.TotalNumberOfPages);

        return Content(HttpStatusCode.OK, objects);
}

//Feels hacky
private PageLinkInfo SetupPagingLinks(UrlHelper url, int page, int pageSize, int totalPages)
{
    PageLinkInfo linkInfo = new PageLinkInfo(); 

    var qs = Url.Request.RequestUri.ParseQueryString();

    //Set default params if missing
    if (qs["page"] == null)
    {
        qs.Set("page", page.ToString());
    }
    if (qs["pageSize"] == null)
    {
        qs.Set("pageSize", pageSize.ToString());
    }

    var baseUrl = Url.Request.RequestUri.GetLeftPart(UriPartial.Path);
    linkInfo.CurrentPageUrl = $"{baseUrl}?{qs}";

    //First page
    qs.Set("page", "1");
    linkInfo.FirstPageUrl = $"{baseUrl}?{qs}";

    //Last page
    qs.Set("page", $"{totalPages}");
    linkInfo.LastPageUrl = $"{baseUrl}?{qs}";

    //Next page
    if (page + 1 <= totalPages)
    {
        qs.Set("page", $"{page + 1}");
        linkInfo.NextPageUrl = $"{baseUrl}?{qs}";
    }
    else
    {
        linkInfo.NextPageUrl = null;
    }

    //Previous page
    if (page - 1 >= 1)
    {
        qs.Set("page", $"{page - 1}");
        linkInfo.PreviousPageUrl = $"{baseUrl}?{qs}";
    }
    else
    {
        linkInfo.PreviousPageUrl = null;
    }

    return linkInfo;
}

Service.cs

public async Task<PagedResults<Object>> GetObjectsAsync(string id, IEnumerable<ConnectionStatus> connectionStatus, int page, int pageSize,
    string orderBy, bool ascending)
{
    var objects = await Repository.SearchForAsync(object => object.id == id && connectionStatus.Contains(object.ConnectionStatus));

    var result = PageObjects(objects, page, pageSize, orderBy, ascending);

    return result;
}

private PagedResults<Object> PageObjects(IEnumerable<ConsumerConnectionGetDto> connections, int page, int pageSize, string orderBy, bool ascending)
{
    switch (orderBy) {
        case "dateConnected":
            connections = connections.OrderBy(x => x.DateConnected).Reverse();
            break;
        case "lastCommunication":
            connections = connections.OrderBy(x => x.LastCommunication).Reverse();
            break;
        case "name":
            connections = connections.OrderBy(x => x.Enterprise.Name);
            break;
        default:
            connections = connections.OrderBy(x => x.Enterprise.Name);
            break;
    }

    if (!ascending) {
        connections = connections.Reverse();
    }

    var skipAmount = pageSize * (page - 1);
    var totalNumberOfRecords = connections.Count();
    var mod = totalNumberOfRecords % pageSize;
    var totalPageCount = (totalNumberOfRecords / pageSize) + (mod == 0 ? 0 : 1);

    var results = connections
        .Skip(skipAmount)
        .Take(pageSize)
        .AsQueryable();

    return new PagedResults<Object>
    {
        Results = results,
        PagingMetaData = new PagingMetaData()
        {
            CurrentPageNumber = page,
            PageSize = pageSize,
            TotalNumberOfRecords = totalNumberOfRecords,
            TotalNumberOfPages = totalPageCount,
            PageLinkInfo = new PageLinkInfo()
            {
                CurrentPageUrl = "",
                FirstPageUrl = "",
                LastPageUrl = "",
                NextPageUrl = "",
                PreviousPageUrl = ""
            }
        }

    };
}

PagedResults.cs

public class PagedResults<T>
{
    [JsonProperty(PropertyName = "_metadata")]
    public PagingMetaData PagingMetaData { get; set; }

    [JsonProperty(PropertyName = "results")]
    public IEnumerable<T> Results { get; set; }
}

public class PagingMetaData
{
    [JsonProperty(PropertyName = "currentPageNumber")]
    public int CurrentPageNumber { get; set; }

    [JsonProperty(PropertyName = "pageSize")]
    public int PageSize { get; set; }

    [JsonProperty(PropertyName = "totalNumberOfPages")]
    public int TotalNumberOfPages { get; set; }

    [JsonProperty(PropertyName = "totalNumberOfRecords")]
    public int TotalNumberOfRecords { get; set; }

    [JsonProperty(PropertyName = "links")]
    public PageLinkInfo PageLinkInfo { get; set; }
}

public class PageLinkInfo
{
    [JsonProperty(PropertyName = "nextPageUrl")]
    public string NextPageUrl { get; set; }
    [JsonProperty(PropertyName = "previousPageUrl")]
    public string PreviousPageUrl { get; set; }
    [JsonProperty(PropertyName = "firstPageUrl")]
    public string FirstPageUrl { get; set; }
    [JsonProperty(PropertyName = "lastPageUrl")]
    public string LastPageUrl { get; set; }
    [JsonProperty(PropertyName = "currentPageUrl")]
    public string CurrentPageUrl { get; set; }
}

Any advice or comments would be appreciated.

Answer 1

The intention

When I have all matching objects - I pass them to a "Paging" method within the service (I think I have to do this in the service rather than the repository because a generic repo paging method won't work across some resources e.g "orderBy" a property nested within a class in an aggregate object).

I'm apprehensive of your design decision here. Pagination serves two purposes:

1. Reducing the data retrieved from the database.
2. Reducing the clutter in the frontend by showing too much data at once (browsers can choke on too much HTML)

As far as I'm aware, 1 is the predominant reason to implement pagination; which you've effectively disabled by only paging after retrieving the data.

This isn't me telling you that you're wrong; but rather getting you to ask yourself whether you're okay with only implementing pagination for the frontend (since you're not gaining anything in the backend), and asking yourself if there's any reason to involve the backend in the pagination process.

Because if you're okay with no backend benefits to pagination, I would suggest that you load all the data into the webpage, and use client side pagination (i.e. simply making the other pages invisible).
This effectively removes the need to resend data between the front- and backend. And, as a plus, client-side pagination is easier to implement than a communication between back and frontend.

Currently, if you have 100 items in total and a page size of 10, and the user needs something on the 9th page, that means that you're going to be loading the 100 items 9 times.

TL;DR

If you're already okay with not having the backend benefits of pagination; you should really consider if you need to involve the backend with the pagination process at all. Not involving the backend and instead opting for client-side pagination has many benefits:

• No repeated data retrieval (of the unpaginated set) for every page change.
• Easier to implement in Javascript, without needing development on the backend.
• Better user experience, as page changes can happen as fast as the browser can render the HTML; without having to wait for a web request to complete.
• Filtering can still trigger a reload of the data if you want it to. So even if your unpaginated data set is filtered, you can set it up so that a user can locally browse between pages with no backend call, but if they change the filters, then they need to reload the data (or the frontend does it automatically for them).

---

The implementation

But this is still a code review.

Use clear variable names.

var qs = Url.Request.RequestUri.ParseQueryString();

//Set default params if missing
if (qs["page"] == null)
{
    qs.Set("page", page.ToString());
}

qs is not a clear name. It makes sense to you now that this is shorthand for "query string", but that wasn't obvious to me. Understand that we approach the code from opposite sides:

• You wrote the code because you knew what needed to be done. You "explained" to the compiler that the application needs to handle the query string.

In your thoughts, you did query string => qs, which makes sense.

• I read the code because I want to learn what needs to be done. The code needs to be able to explain to me that you want to work with the query string.

In my thoughts, this requires me to do qs => query string, which is not as obvious.

Secondly, the name is not even correct! A query string is a string represenation of a key/value dataset. qs is not a string anymore, it's the array that was converted from what used to be a string.

var queryParams = Url.Request.RequestUri.ParseQueryString();

//Set default params if missing
if (queryParams["page"] == null)
{
    queryParams.Set("page", page.ToString());
}

Much clearer, no?

Note: I used Params instead of Parameters. It seems like I'm making the same mistake here, but I somewhat disagree. The shortening here seems obvious, and it keeps the code a bit nicer.
If you disagree, that's fine too. It's a matter of preference, really.

Stringly typed is badly typed.

switch (orderBy) {
    case "dateConnected":
        connections = connections.OrderBy(x => x.DateConnected).Reverse();
        break;

Since you're already ensuring that your strings can only be picked from a manually maintained list of options, you're better off using an enum:

public enum OrderByColumn { None, DateConnected, LastCommunication, Name }

switch (orderBy) {
    case OrderByColumn.DateConnected:
        connections = connections.OrderBy(x => x.DateConnected).Reverse();
        break;

You get intellisense and, on top of that, you ensure that typos will lead to compile time errors rather than runtime exceptions (or unexpected behavior).

Note that there is an even better approach, if you're willing to use a generic method and expressions. You can effectively use x => x.DateConnected as a method parameter by itself (without needing an enum to map to, while still providing the same benefits I mentioned before).
However, that does drastically increase the code complexity, which seems out of scope for your intentions here.

Sidenote

if (!ascending) {
    connections = connections.Reverse();
}

This irks me. You're effectively doing two jobs when your want a descending order. This can be easily avoided if you have the boolean influence deciding between using Orderby and OrderByDescending.

However, due to your current switch-based approach to ordering; I don't suggest adding if else statements to every switch case. However, if you decide to use the expressions I mention earlier, then you can implement this in a better way.

Improved formula

var mod = totalNumberOfRecords % pageSize;
var totalPageCount = (totalNumberOfRecords / pageSize) + (mod == 0 ? 0 : 1);

This can be optimized a bit, if you simply round up the (decimal) page count:

var totalPageCount = Math.Ceiling((float)totalNumberOfRecords / pageSize);

Same outcome, negligible performance difference, but it yields easier readable code.

I object to Object.

Object is a poorly chosen name. It obviously clashes with object. Even if the compiler allows you to; you shouldn't rely on capitalization to distinguish between two completely different types.

You didn't post your actuall class definition. Is Object a generic parameter type? If so, you should at the very least be using TObject. But I would still avoid the "object" name altogether and simply use either T or TEntity.

In most cases where I've seen developers want to refer to their own classes as "objects", "entity" has been a much better naming decision that avoids ambiguity.

Small refactor

if (page + 1 <= totalPages)

if (page - 1 >= 1)

Can quickly be simplified:

if(page < totalPages)

if (page > 0)

I agree

//Feels hacky
private PageLinkInfo SetupPagingLinks(UrlHelper url, int page, int pageSize, int totalPages)

When you look at what this method does, it's effectively a constructor method. So make it an actual constructor method:

public class LinkInfo
{
    public LinkInfo(UrlHelper url, int page, int pageSize, int totalPages)
    {
        //...
    }
}

Personally, I disagree with the fact that you format all URLs individually. Most of this can be done by the frontend. Usually, all you need to pass is the following:

• Total pages
• (optional): If you want to show something like "Showing item 21-40 out of 100 items, you will also need to pass the page size and total item count. If you don't need to show it, you don't need to pass it.

The current page number can be read from the page's query string. If absent, assume a default value of 1 (note that I'm using a 1-based page index because that's how users want to work with page numbers.)

The frontend can figure everything out from there.

var shouldShowFirstAndPreviousPageButton = (pageNumber > 1);
var firstPageButtonLink = url + 1;
var previousPageButtonLink = url + (pageNumber - 1);

var shouldShowNextAndLastPageButton = (pageNumber < totalPages);
var nextPageButtonLink = url + (pageNumber + 1);
var lastPageButtonLink = url + totalPages;

This code is oversimplified so that it focuses on the important parts. Adding a query parameter correctly takes a bit more of syntax but is not all that relevant and clutters the example logic.

These are all values that can easily be calculated, I see no reason to have the backend preformat them.

Note that the URL concatenation wouldn't be necessary if you're using ajax calls; you can simply pass the filters + desired page number as a JSON object in javascript. In that case, all you need to know is the base url (which is the same as the current page's url anyway; so it should again not really be preformatted by the backend).

This reduces the data footprint. The frontend can figure everything out by itself, other than the total amount of pages (and, optionally, the total amount of items and the page size).

Note: If you switch to client-side pagination, as mentioned in the first part of this answer, this becomes even simpler. Everything can be done based on the set of items that you've passed to the frontend. You won't actually need any paging metadata.