Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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<Post> posts = repository.All();
    PagedList<Post> 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.

share|improve this question
    
Do you seriously want to do pagination on an IEnumerable<T>? That means if you want to display page 10, it needs to materialize 9 pages of items which you're not interested in. –  CodesInChaos Aug 25 at 12:44
    
I will use IQueryable<T> when the need presents itself. –  Caster Troy Aug 27 at 11:23

1 Answer 1

up vote 11 down vote accepted

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.

share|improve this answer
1  
Mat. My dude. I really wish you would have just dropped a comment real quick to clarify instead of making so many (incorrect) assumptions. As a result, all but the last few paragraphs are irrelevant to me. The answer has some nice factoids that might be useful to some readers but now if I edit my post, I will invalidate your answer. I am partially to blame for using var so sorry. In all actuality the Paginate extension returns a PagedList! You can figure it out from there. The last few paragraphs are helpful to me though so thanks. I will have a think about which approach to take. –  Caster Troy Aug 24 at 14:19
    
Oh.. how about I scratch the first two strikes off that answer then? –  Mat's Mug Aug 24 at 14:46
    
Sound. I tweaked my question. You can edit accordingly if you want to. –  Caster Troy Aug 24 at 14:49
    
Great! Edited accordingly :) –  Mat's Mug Aug 24 at 14:52
    
Thanks Mat's Mug :3 –  Caster Troy Aug 29 at 17:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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