-1
\$\begingroup\$

I've struggled with this for quite some time. Today I finally wrote the following code.

The ViewModel contains an int property which later tells the view how many pages the data has been split into.

The controller splits the data by taking a specified amount of rows and, in the event of paging, splits by pageNumber * recordsPerPage

Take a look:

The ViewModel

public class ThreadPostsViewModel
{
    public Thread Thread { get; set; }
    public List<Post> Posts { get; set; }
    public int Pages { get; set; }
}

The Controller

private int PostsPerPage = 10;

public ActionResult Thread(int id, int page = 1)
{
    using (OrtundEntities Db = new OrtundEntities())
    {
        // get the thread and its parent data (parent for breadcrumbs)
        var Thread = Db.Threads.Include(t => t.Title).FirstOrDefault(x => x.Id == id);

        // create a list for the Posts
        List<Post> Posts = new List<Post>();

        // select based on paging
        if (page == 1)
            // no paging has happened, get the first set of records
            Posts = Db.Posts.Include(x => x.User).Where(x => x.ThreadId == id).OrderByDescending(x => x.Date).Take(PostsPerPage).ToList();
        else
            // we're on a new page. Skip however many rows we've already seen
            Posts = Db.Posts.Include(x => x.User).Where( x=> x.ThreadId == id).OrderByDescending(x => x.Date).Take(PostsPerPage).Skip(PostsPerPage * page).ToList();

        // create and return the view model
        ThreadPostsViewModel Model = new ThreadPostsViewModel
        {
            Thread = Thread,
            Posts = Posts,
            Pages = Posts.Count / PostsPerPage
        };

        return View(Model);
    }
}

The View

@model Ortund.Models.ThreadPostsViewModel
<div class="paging">
    @for (int i = 1; i < Model.Pages; i++)
    {
        string Url = String.Format("/View/Thread/{0}?page={1}", Model.Thread.Id, i);

        <a href="@Url">@i</a>
    }
</div>
<div class="posts-list">
    @foreach (var Post in Model.Posts)
    {
        <div class="post" id="@Post.Id">

        </div>
    }
</div>

In this code, assuming 300 posts are selected from the database and 10 posts are specified per page, then there should be 30 pages.

\$\endgroup\$

closed as off-topic by alecxe, Vogel612, Mathias Ettinger, Graipher, Incomputable Aug 22 '17 at 13:46

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – alecxe, Vogel612, Mathias Ettinger, Graipher, Incomputable
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I'm voting for closing becase this code is broken and doesn't do what it's supposed to do. \$\endgroup\$ – t3chb0t Sep 5 '16 at 4:34
  • \$\begingroup\$ That's a lousy reason to just throw the question away... \$\endgroup\$ – Ortund Sep 5 '16 at 10:33
  • 2
    \$\begingroup\$ If you read the rules you'd know that only working code is subject to review. \$\endgroup\$ – t3chb0t Sep 5 '16 at 10:34
  • 3
    \$\begingroup\$ On Code Review it is our policy to review code that fully works as intended. Please read How to Ask and help center. \$\endgroup\$ – janos Aug 22 '17 at 7:32
1
\$\begingroup\$

Your naming is incosistent. For some variables you use var Thread and for others the full name ThreadPostsViewModel Model. Besides we name local variables in camelCase.


You can greatly shorten this query by not repeating yourself and create the result model at once.

Did you test it? Because this contains several bugs and doesn't do what you think it does.

If you Take(PostsPerPage) and then .Skip(PostsPerPage * page) then you'll have nothing left.

This should be the other way around and if for the Skip you write PostsPerPage * (page - 1) then for the page 1 you'll skip 0 records so you don't need two queries.

Also this Posts.Count / PostsPerPage will always return 1 - it should be Db.Pages.Count / PostsPerPage.

var result = new ThreadPostsViewModel
{
    Thread = Db.Threads
        .Include(t => t.Title)
        .FirstOrDefault(x => x.Id == id),
    Posts = Db.Posts
        .Include(x => x.User)
        .Where( x=> x.ThreadId == id)
        .OrderByDescending(x => x.Date)
        .Skip(PostsPerPage * (page - 1))
        .Take(PostsPerPage),
    Pages = Db.Pages.Count / PostsPerPage
};
\$\endgroup\$
  • \$\begingroup\$ Not to argue about the naming conventions but... The PascalCasing convention, used for all identifiers except parameter names, capitalizes the first character of each word (including acronyms over two letters in length). This from msdn.microsoft.com/en-us/library/ms229043(v=vs.110).aspx \$\endgroup\$ – Ortund Sep 5 '16 at 9:19
  • \$\begingroup\$ Also Db.Pages.Count isn't actually a thing in my data model. I'm deriving the amount of pages from the total number of posts / PostsPerPageso if I have 420 posts, and 10 PostsPerPage, then Pages should work out to 420 / 10 = 42 although it just occurred to me that I should add a page in cases where this division yields a remainder (so 426 posts would result in 43 page where page 43 has only 6 posts on it) \$\endgroup\$ – Ortund Sep 5 '16 at 9:57

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