Pagination implementation for a blog system

Question

Last night I was working on a project that I've embarked upon as a learning exercise. I decided to add a pagination feature to my blog system. I made an attempt at it and got close... but I decided to undo my changes and I searched online for other 'better' solutions. I found this example which uses PagedList.Mvc NuGet and this example from asp.net which uses the same package.

Long story short, I decided that before I install another package I would make one more attempt at my own implementation and I was successful... so, on to my question.

Have I missed something important in my implementation that should make me want to reconsider my approach?

Here's my relevant Action in my HomeController.cs:

public ActionResult Index(int num = 0)
{
    var postsPerPage = 3;
    ViewBag.startNum = num;

    using (UnitOfWork uwork = new UnitOfWork())
    {
        IEnumerable<Post> posts = uwork.PostRepository.GetAll().ToList();

        int totalPosts = posts.Count();
        ViewBag.pages = totalPosts / postsPerPage;
        int skip = num * postsPerPage;


        return View(posts.Skip(skip).Take(postsPerPage));
    }
}

and this is the relevant code from my Index.cshtml:

<ul class="pagination">
    <li>@Html.ActionLink("Home", "Index", "Home", new { num = 0 }, null)</li>
    @for (var i = 1; i < ViewBag.pages + 1; i++)
    {
        var cur = i.ToString();
        var name = i + 1;
        <li>@Html.ActionLink(name.ToString(), "Index", "Home", new { num = cur }, null)</li>

        if (i >= 10) { break; }

    }
</ul>

Note: I know there's going to be a bit more logic required in order to account for the remainder of pages after they are divided. But that's not my biggest concern as of yet.

Answer 1

int num

What does num represent exactly? Number of pages? Number of posts per page? Current page number? Properly naming the variable would help a lot in clarity here.

---

var postsPerPage = 3;

Turn it into a parameter with default value 3. That way you can always decide later that the user can change this without much trouble.

---

Instead of using 0-based pages and then adding 1 to the presentation, I would consider using the reverse: 1-based pages and subtracting 1 when determining the pages to display. In essence this would turn your loop from i = 2, name becomes unnecessary and in your controller you use int skip = (num - 1) * postsPerPage instead.

---

using (UnitOfWork uwork = new UnitOfWork())
{
    IEnumerable<Post> posts = uwork.PostRepository.GetAll().ToList();

    int totalPosts = posts.Count();
    ViewBag.pages = totalPosts / postsPerPage;
    int skip = num * postsPerPage;


    return View(posts.Skip(skip).Take(postsPerPage));
}

ViewBag.pages (note the incorrect naming convention) indicates to me a collection of pages. In your case however, a more appropriate name would be AmountOfPages.

---

I feel like there's room for an optimization here: what if you have 100.000 pages? Or even just 100? You will load all your items each time someone visits the frontpage and you're only doing this to show the user how many pages of items there are. In fact you're not even doing that: you won't show them past 10 pages anyway!

I would argue that maybe you should just use two queries:

var totalPosts = uwork.PostRepository.GetAll().Count() 

which will just perform a select count(*) from posts at the database side and another query that gives you your pages:

var pages = uwork.PostRepository.GetAll().Skip(skip).Take(postsPerPage)

Another way to keep performance in mind is to use

uwork.PostRepository.GetAll().Take(postsPerPage * 10).ToArray();

This will you give

1. The amount of posts (the exact amount being pages.Length)
2. The concrete posts (being pages.Skip(skip).Take(postsPerPage))

The benefit of this is that it will only call the database once (network latency is one of your demons) with the downside being that it retrieves more items than you're interested in.

---

Instead of looping to ViewBag.pages + 1 and possibly breaking out at i >= 10, I would just loop until Math.Min(ViewBag.Pages + 1, 10).