HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Somewhat esoteric if statement in a paginated feed

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
statementsomewhatfeedesotericpaginated

Problem

I am working on the home page of a website that will have a paginated feed (much like a blog's home page).

One of the requirements is that when a user navigates to a non-existent page, he or she will be redirected to the last available page. For example, when the user navigates to a non-existent page such as page number 500, they will be redirected to the highest (last available) page number which might be something sensible like 10.

The code to fulfill this requirement easy enough looks like this:

private const int PageSize = 5; 
public ActionResult Index(int pageNumber = 1)
{
    IEnumerable posts = repository.All();
    PagedList page = posts.Paginate(pageNumber, PageSize);

    if (page.Count == 0 && pageNumber != 1)
    {
        return RedirectToAction("Index", new { pageNumber = page.TotalPageCount });
    }

    return View(page);
}


My problem with this code is the following expression:

page.Count == 0 && pageNumber != 1


Personally I find this code hard to reason about (and I am the person who wrote it!). How can I make the meaning of this expression clearer? Is there an alternative, more readable way of implementing this logic perhaps?

The condition begins by evaluating whether the requested page has any posts - if the contents of the page are empty, the page effectively does not exist. The condition then checks whether the user is attempting to access the home page (the first page) or not. This check is important because if the database is empty, the user still needs to be able to access the first page - so that the view can display a nice error message.

Solution

You're checking that "page does not contain any posts and page number is not one".

Reversing the condition is often the easiest way to clarify it. The inverse condition would be "page contains at least one post, or page number is one".

I would make it look like this:

if (pagePosts.Any() || pageNumber == 1)
{
    return View(pagePosts);
}
else
{
    return RedirectToAction("Index", new { pageNumber = pagePosts.TotalPageCount });
}


Notice page renamed to pagePosts, also for clarity: it's not a page, it's the posts on the page that was requested. pagePosts is less ambiguous I find.

Code Snippets

if (pagePosts.Any() || pageNumber == 1)
{
    return View(pagePosts);
}
else
{
    return RedirectToAction("Index", new { pageNumber = pagePosts.TotalPageCount });
}

Context

StackExchange Code Review Q#60908, answer score: 11

Revisions (0)

No revisions yet.