WebApi Get with paging

Question

How can I improve this code?

public virtual HttpResponseMessage Get(int pageNo, int pageSize)
{
    pageNo = pageNo > 0 ? pageNo - 1 : 0;
    pageSize = pageSize > 0 ? pageSize : 0;

    int total = repository.Table.Count();
    int pageCount = total > 0 ? (int)Math.Ceiling(total / (double)pageSize) : 0;

    var entity = repository.Table.OrderBy(c => c.ID).Skip(pageNo * pageSize).Take(pageSize);

    if (entity.Count() == 0 || entity == null)
    {
        var message = string.Format("{0}: No content", GenericTypeName);
        return ErrorMsg(HttpStatusCode.NotFound, message);
    }

    var response = Request.CreateResponse(HttpStatusCode.OK, entity);
    response.Headers.Add("X-Paging-PageNo", (pageNo + 1).ToString());
    response.Headers.Add("X-Paging-PageSize", pageSize.ToString());
    response.Headers.Add("X-Paging-PageCount", pageCount.ToString());
    response.Headers.Add("X-Paging-TotalRecordCount", total.ToString());

    return response;
}

Answer 1

Let's start with the BUG.

if (entity.Count() == 0 || entity == null)

If entity is null, then you'll get a NullReferenceException when Count is called, so there's currently no possible way the second half of this statement will ever be called. I think you meant to do this.

 if (entity == null || entity.Count == 0)

Which works, but isn't great. I would prefer "not Any" here. I find it's good practice to get into the habit of using Any. It's just more readable IMO.

if ( entity == null || !entity.Any() )

---

To clarify. Any() returns as soon it finds an element, whereas Count has to iterate over the entire Enumerable before it returns.

Note that this isn't true for List. Any() still returns early when called on a List, but so will Count because a list keeps track of how many items it has as they're added/removed. Enumerables do not already "know" how big they are. They must be iterated to get a count.

---

Now let's back up a bit and stop your Linq from scrolling off the screen.

var entity = repository.Table
                 .OrderBy(c => c.ID)
                 .Skip(pageNo * pageSize)
                 .Take(pageSize);

Maybe it's because I "grew up" with a language where everything is passed ByRef by default (.Net passes ByVal by default), but I don't like assigning values to arguments.

public virtual HttpResponseMessage Get(int pageNo, int pageSize)
{
    pageNo = pageNo > 0 ? pageNo - 1 : 0;
    pageSize = pageSize > 0 ? pageSize : 0;

I'd introduce another variable, but I'm having a hard time thinking of good names at the moment. I'd also consider extracting the logic into private methods to clarify the intent. It doesn't matter that you're unlikely to ever call them from anywhere else, some well named methods can really clarify what code is doing. You can always look inside them if you're interested in the implementation, but at this level, I don't care how we're calculating these numbers, I just care that we are.

public virtual HttpResponseMessage Get(int pageNo, int pageSize)
{
    pageNo = NormalizePageNo(pageNo);
    pageSize = NormalizePageSize(PageSize);

Here's another great opportunity to give logic a name.

int pageCount = total > 0 ? (int)Math.Ceiling(total / (double)pageSize) : 0;

Mmmm hmmm... Now what is that doing exactly?

If the total number of records is greater than zero,
Then return the percentage of total divided by page size, rounded up
Else return Zero.

Again, these are implementation details. At the current level of abstraction, we just want to know

int pageCount = CalculatePageCount(total, pageSize);