6
\$\begingroup\$

router/article.js

// =============================== GET / WITH PAGINATION ==============================

router.get('/?:page', async (req, res) => {
  const { page } = req.params; // current Page
  const { size } = req.params; // items per page
  const limit = size || 5; // if size isn't specified, set size to 5
  const offset = page ? (page - 1) * limit : 0; // skip previous items
  const navBarIndexLimit = 4; // how many page indexes to show in navigation bar like < 1 2 3 4 >
  const navBarIndexOffset = Math.floor(page / navBarIndexLimit); // offset previous indexes like < 5 6 7 8 >
  const navBarIndexArray = []; // index for navigating pages in view

  // fetch articles
  const articles = await Article.findAndCountAll({
    include: User,
    raw: true,
    nest: true,
    limit,
    offset,
  });
  const totalPage = Math.ceil(articles.count / limit);

  // if at last page, show previous indexes
  if (page >= totalPage) {
    for (let i = 0; i < navBarIndexLimit; i += 1) {
      const prevPage = page - i;
      navBarIndexArray.push(page - i);
      if (prevPage <= 0) {
        break;
      }
    }
    navBarIndexArray.sort();
  }

  // set index bar
  else {
    for (let i = 0; i < navBarIndexLimit; i += 1) {
      const nextPage = i + navBarIndexOffset * navBarIndexLimit;
      if (nextPage > totalPage) {
        break;
      }
      navBarIndexArray.push(nextPage + 1);
    }
  }


  articles.index = navBarIndexArray;
  res.render('article/article_home.hbs', { articles });
});

article.home.hbs

<div class="ui pagination menu">
  <a class="item">
    <-
  </a>
  {{#each articles.index as |key|}}
  <a class="item" href="/article/{{key}}">
    {{key}}
  </a>
  {{/each}}
  <a class="item">
    ->
  </a>
</div>

I've managed to paginate my articles and while it is working, I have a strong feeling that I made the pagination navigation bar part extremely complicated and I'm sure there would be a better way.

Using Express, Sequelize and Handlebars

\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

Error handling You await inside an async function, but without a try/catch surrounding it. So, if Article.findAndCountAll rejects, you'll get an unhandled rejection (which is deprecated in Node) and no response will be sent.

Default destructuring and variable names

const { page } = req.params; // current Page
const { size } = req.params; // items per page
const limit = size || 5; 

simplifies to

const { page, size = 5 } = req.params;

and using the size variable. Though, if possible, change the param and variable name to itemsPerPage; it's much more representative of what the variable contains than size. currentPage would also be a bit more precise. Similarly, totalPage, since it contains a number, would sound better as totalPages.

Sorting bug You have:

navBarIndexArray.sort();

This will sort lexiographically, not numerically. For example, it could produce an output of

[1, 11, 2]

Compare numerically instead, eg .sort((a, b) => a - b).

Pagination Or, even better, just insert the indicies in order to begin with.

Your current results don't seem quite natural. When near the end, I'd expect the nav to be full of all nearby pages: eg, when on page 5 of 6, I'd expect 3 4 5 6, not just 4 5 6. Implementing this will require a bit of extra logic, though.

Rather than two loops which do something very similar, identify the last page to display in the nav, then subtract by navBarIndexLimit to identify the first page to display in the nav, then iterate over it. See comments in snippet below:

const navBarIndexLimit = 4;
// currentToLastPageOffset: Amount to add to current page to get last page
// (eg: current page 4 results in last page 6, first page 3)
const currentToLastPageOffset = navBarIndexLimit - 2;
const makeNavbarIndexArr = (currentPage, totalPages) => {
  const lastPage = Math.min(
    totalPages,
    currentPage <= 1
      ? navBarIndexLimit - 1 // If on 0th or 1st page, set last page to 3
      : currentPage + currentToLastPageOffset
  );
  const firstPage = Math.max(lastPage - (navBarIndexLimit- 1), 0);
  // I prefer to create numeric arrays all at once with Array.from:
  return Array.from(
    { length: lastPage - firstPage + 1 },
    (_, i) => firstPage + i
  );
  // But you could also use a for loop and push if you find it easier to read
};
console.log(makeNavbarIndexArr(0, 9));
console.log(makeNavbarIndexArr(1, 9));
console.log(makeNavbarIndexArr(5, 9));
console.log(makeNavbarIndexArr(8, 9));
console.log(makeNavbarIndexArr(9, 9));
.as-console-wrapper {
  max-height: 100% !important;
}

Note that the above takes zero-indexed parameters and returns an array that's zero-indexed, though it's trivial to tweak it to be one-indexed instead if needed.

It's a reasonable amount of code, but the logic that needs to be implemented is somewhat complicated, so there's no avoiding at least some of it. While it could be condensed somewhat (for example, by removing the currentToLastPageOffset variable), that would make it harder to understand.

I highly recommend keeping the function in the snippet above - don't inline it into your page route, so as to clearly separate the calculation logic from the main route logic.

router.get('/?:page', (req, res) => {
    const { currentPage, itemsPerPage = 5 } = req.params;
    const offset = currentPage ? (currentPage - 1) * itemsPerPage : 0; // skip previous items
    Article.findAndCountAll({
        include: User,
        raw: true,
        nest: true,
        limit: itemsPerPage,
        offset,
    })
        .then((articles) => {
            const totalPages = Math.ceil(articles.count / itemsPerPage);
            articles.index = makeNavbarIndexArr(currentPage, totalPages);
            res.render('article/article_home.hbs', { articles });
        })
        .catch((error) => {
            // Handle errors, send error response
        });
});
\$\endgroup\$
2
  • \$\begingroup\$ how about adding if (articles.index.includes(0)) { articles.index = articles.index.map((i) => i + 1); } to remove zero index? and thanks for your great insight! \$\endgroup\$
    – viviet
    Oct 12, 2020 at 21:53
  • \$\begingroup\$ If you want it to be 1-indexed, you'd want to map the array unconditionally, no matter whether it includes 0 or not \$\endgroup\$ Oct 12, 2020 at 22:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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