2
\$\begingroup\$

Basic paging function, is there logical problems with the JavaScript part?

very basic paging function.
I want to know if there are some problems in the logic of the code. I feel the JS code is a bit bloated.

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <meta http-equiv="X-UA-Compatible" content="ie=edge">
  <title>Go Coding!</title>
  <style>
    * {
      margin: 0;
      padding: 0;
      border: none;
      font-size: 12px;
      list-style: none;
      text-decoration: none;
    }

    body {
      padding: 20px;
    }

    .title {
      font-size: 36px;
      color: #f80;
    }

    .title>small {
      font-size: 24px;
      color: #ccc;
      vertical-align: 4px;
    }

    .tip {
      color: #999;
    }

    .emp {
      color: #f80;
    }

    .list>li {
      padding: 6px;
      border-bottom: 1px solid #eee;
    }

    .pagination {
      padding: 4px;
    }

    .pagination a {
      display: inline-block;
      padding: 4px 8px;
      color: #00c;
    }

    .pagination a.active {
      font-weight: bold;
      color: #f80;
      background-color: #eee;
    }

    .pagination .tip {
      margin: 0 0 0 10px;
    }
  </style>
</head>
<body>
  <ul class="list"></ul>
  <section class="pagination"></section>
  <script>
    let data = {
      "list": [
        { "id": 1, "title": 2001 },
        { "id": 2, "title": 2002 },
        { "id": 3, "title": 2003 },
        { "id": 4, "title": 2004 },
        { "id": 5, "title": 2005 },
        { "id": 6, "title": 2006 },
        { "id": 7, "title": 2007 },
        { "id": 8, "title": 2008 },
        { "id": 9, "title": 2009 },
        { "id": 10, "title": 2010 },
        { "id": 11, "title": 2011 },
        // { "id": 12, "title": 2012 },
        // { "id": 13, "title": 2013 },
        // { "id": 14, "title": 2014 },
        // { "id": 15, "title": 2015 },
        // { "id": 16, "title": 2016 },
        // { "id": 17, "title": 2017 },
        // { "id": 18, "title": 2018 },
        // { "id": 19, "title": 2019 },
        // { "id": 20, "title": 2020 },
        // { "id": 21, "title": 2021 },
      ]
    }

    let list = data.list;
    console.log(`Data Length: ${list.length}`);

    pagination({
      "current-page": 1,
      "page-size": 2,
    });

    function pagination(par) {
      let pagination = document.querySelector(".pagination");
      let listBox = document.querySelector(".list");
      let currentPage = par["current-page"];
      let pageNums = par["page-nums"] || 5;
      let pageSize = par["page-size"];
      let num = data.list.length;
      let totalPage = 0;

      if (num / pageSize > parseInt(num / pageSize)) {
        totalPage = parseInt(num / pageSize) + 1;
      } else {
        totalPage = parseInt(num / pageSize);
      }

      let sliceStart = (currentPage - 1) * pageSize;
      let sliceEnd = currentPage * pageSize;
      sliceEnd = sliceEnd > num ? num : sliceEnd;

      let dataSlice = data.list.slice(sliceStart, sliceEnd);
      let listText = "";
      for (let item of dataSlice) {
        listText += `
          <li>
            <h5 class="title">
              <small>${item.id} - </small>${item.title}
            </h5>
          </li>
        `;
      }
      listBox.innerHTML = listText;

      let htmlText = "";
      if (currentPage > 1) {
        htmlText += `
          <a href="#" onClick="pagination({'current-page': ${currentPage - 1}, 'page-size': ${pageSize}})">
            &#8592;
          </a>
        `;
      }

      let start = currentPage - Math.floor(pageNums / 2);
      let end = currentPage + Math.floor(pageNums / 2);

      if (end < pageNums) {
        end = pageNums;
      }

      if (end > totalPage) {
        end = totalPage;
      }

      if (end - start != pageNums - 1) {
        start = end - pageNums + 1;
      }

      if (start < 1) {
        start = 1;
      }

      for (let i = start; i <= end; i++) {
        htmlText += `
          <a class="${currentPage === i ? 'item-link active' : 'item-link'}" href="#" data-index="${i}" onClick="pagination({'current-page': ${i}, 'page-size': ${pageSize}})">
            ${i} 
          </a>
        `;
      }

      if (currentPage < totalPage) {
        htmlText += `
          <a href="#" onClick="pagination({'current-page': ${currentPage + 1}, 'page-size': ${pageSize}})">
            &#8594;
          </a>
        `;
      }

      htmlText += `
        <span class="tip">
          <b class="emp">${currentPage}</b> / ${totalPage}
        </span>
      `;
      pagination.innerHTML = htmlText;
    }
  </script>
</body>
</html>

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

UX

Great job on the end product! There are no distracting CSS animation frills and everything appears to work smoothly.

Consider disabling the forward/backward arrows on the first and last pages instead of removing them entirely. This way, the numbers are always in a fixed place and the user won't accidentally click the wrong thing or have to reposition their cursor after the page numbers shift unexpectedly after a click.

Use <strong> instead of <b>. HTML is for semantics/structure, not style/presentation.

Consider <button> instead of <a> for similar reasons. The <a href="#"> leaves an ugly # in the browser URL (yeah, you can use javascript:void(0) but that's also obtrusive). These buttons could be in a list.

Use const instead of let

Instead of using let everywhere, prefer using const everywhere. Yes, it's two more characters to type but it has at least three benefits:

  1. const prevents a class of bugs related to inadvertent reassigments. Take any increase in guarantees you can get for free.

  2. const adds expressiveness to the code: when you do use let, it communicates to the reader that you intend to reassign the variable, such as in a loop counter. If you apply let across the board, a reader can't distinguish effectively-final variables (like pageNums) and intended-to-be-reassigned dynamic variables (like currentPage).

  3. If you make an effort to prioritize const throughout your code, yet you notice you're using let excessively, it's a useful code smell indicating over-reliance on mutability. This applies to this program. Frequently, you declare some numerical variable with let, then mutate it in a series of branches to clamp to max/min bounds and so forth. It's not obvious what sort of values they might have at a given program point.

    If you challenge yourself to code without let, you'd have to break this logic into functions. You could use ternaries, but ternaries are OK in my book as long as they're not nested. += can become map/join.

    Refactoring let into const should automatically make your code safer and cleaner.

Style suggestions

  • .title>small should be .title > small so it doesn't look like a single class name.

  • I like the config object parameter for pagination but par isn't the clearest variable name. Consider destructuring par into variables or rename it to config.

  • "current-page" in your pagination config is odd because kebab case isn't used in JS. Prefer currentPage for easy destructuring and style consistency.

  • let list = data.list; can use destructuring const {list} = data;. You don't have to see list twice and it's easier to add more properties in the future.

  • Spread long HTML properties onto multiple lines. Try to stick to a max width of 80-90 characters.

    htmlText += `
      <a class="${currentPage === i ? 'item-link active' : 'item-link'}" href="#" data-index="${i}" onClick="pagination({'current-page': ${i}, 'page-size': ${pageSize}})">
        ${i} 
      </a>
    `;
    

    becomes

    htmlText += `
      <a
         class="${currentPage === i ? 'item-link active' : 'item-link'}" 
         href="#" 
         data-index="${i}"
         onClick="pagination({'current-page': ${i}, 'page-size': ${pageSize}})"
      >
        ${i} 
      </a>
    `;
    
  • Functions should be verbs, classes should be nouns. pagination should be called paginate or createPagination. If it were a class, it'd be Pagination or Paginator.

Avoid monolithic functions

pagination is too large. Try to break it into helper functions for logical subtasks that build HTML or compute start/end bounds.

For example,

let totalPage = 0;

if (num / pageSize > parseInt(num / pageSize)) {
  totalPage = parseInt(num / pageSize) + 1;
} else {
  totalPage = parseInt(num / pageSize);
}

could be a named function. In addition to modularity, parameters and return give a lot of power to avoid reassignments, mutability and ugly control flow.

Beyond that, parseInt(num / pageSize) is repeated 3 times and num / pageSize 4 times. Math.floor communicates what this code is doing better than parseInt.

In this case, I think the whole thing can be replaced with const totalPages = Math.ceil(num / pageSize).

Keep Math.max and Math.min in mind to replace if-else blocks.

Avoid hardcoded globals

The pagination function isn't reusable because it's tied to a special variable data in the external scope. data must have a list property, every item in the list has to have title and id properties and document.querySelector(".pagination") has to exist.

Your intent was probably to make a function so that it could be called as a handler -- basically, bare minimum to operate. This is fine for a toy example but not suitable for a large app.

If the paginator is self-contained, then you can use it as a library, move it to its own module and create as many paginators as you need without worrying about corrupt state.

Tightly-scoped data and loose coupling makes it easy to localize bugs.

Separate responsibility

With pagination, it's typically the backend database that should pick out the subset of records. Typically, your API will have routes for getting a total count of records, then offset and limit parameters, something like /api/posts?offset=30&limit=10.

However, this paginator seems to expect to work with a whole data set, only to pick out and render a page with let dataSlice = data.list.slice(sliceStart, sliceEnd);.

You could pull out the list-rendering logic from the paginator and let the caller deal with it as they see fit. The paginator only needs to fire a page change event. No need to pass the data in, slice it or assume anything else about it.

Use caution with template strings, innerHTML and +=

I'm a huge fan of template strings with innerHTML. It offers an OK chunk of the syntactic power and cleanliness of JSX in vanilla JS.

These tools aren't entirely free lunch, though. One problem is potential performance issues. Setting .innerHTML means the string is parsed and converted to DOM elements under the hood. += can be potentially slow because of Shlemiel the painter's algorithm. We don't have a React diffing algorithm to selectively rerender only specific children.

These aren't immediate issues and engine optimizations make += and .innerHTML as fast as possible, but keep this in mind when profiling if you notice performance problems later.

Unlike JSX, template strings and .innerHTML don't sanitize anything, so be sure to sanitize to prevent XSS attacks if the data isn't trustworthy.

Short of React/Vue/etc, you could try a JSX-like library like htm to solve some of these problems.

Check out existing packages

When reinventing the wheel, it's a good idea to skim libraries on npm and GitHub and see what the state of the art is. This can steer you clear of poor designs and towards good ones up front. There's value in doing it the hard way blindly, but check out other designs afterwards.

For example, react-paginate accepts a click handler for page navigation and doesn't actually render the list. Even if you don't code React, you can probably see how this works on a high level from their demo.

Rewrite suggestion

This isn't an industrial-strength paginator and could benefit from validating parameters, but it seems good enough to illustrate my points.

const paginate = ({
  container,
  totalPages,
  pagesToDisplay,
  startPage,
  onPageChange,
  classes,
  prevText,
  nextText,
}) => {
  const list = (size, cb) => Array(size).fill().map(cb).join("");

  const makePaginationHTML = (currentPage, start) => `
    ${currentPage === 0 ? "" : `
      <a href="#" class="${classes.prev}">${prevText}</a>
    `}
    ${list(pagesToDisplay, (_, i) => `
      <a href="#" class="${classes.item}">${start + i + 1}</a>
    `)}
    ${currentPage === totalPages - 1 ? "" : `
      <a href="#" class="${classes.next}">${nextText}</a>
    `}
  `;

  const addPaginationListeners = (els, currentPage, start) => {
    [...els].forEach((e, i) => {
      start + i === currentPage && e.classList.add("active");
      e.addEventListener("click", () => changePage(start + i));
    });
    container.querySelector("." + classes.prev)
      ?.addEventListener("click", () => changePage(currentPage - 1))
    ;
    container.querySelector("." + classes.next)
      ?.addEventListener("click", () => changePage(currentPage + 1))
    ;
  };

  const renderPagination = currentPage => {
    const half = Math.floor(pagesToDisplay / 2);
    const start = currentPage >= totalPages - half
      ? totalPages - pagesToDisplay
      : Math.max(0, currentPage - half)
    ;
    container.innerHTML = makePaginationHTML(currentPage, start);
    addPaginationListeners(
      container.getElementsByClassName(classes.item), 
      currentPage, 
      start
    );
  };

  const changePage = nextPage => {
    renderPagination(nextPage);
    onPageChange(nextPage);
  };
  changePage(startPage);
};

const renderPages = (currentPage, totalPages) => {
  document.querySelector(".pagination-tip").innerHTML = `
    <strong class="emp">${currentPage + 1}</strong> / ${totalPages}
  `;
};
const renderList = data => {
  document.querySelector(".list").innerHTML = data.map(e => `
    <li>
      <h5 class="title">
        <small>${e.id} - </small>${e.title}
      </h5>
    </li>
  `).join("");
};

(() => {
  const data = {
    list: [
      {id: 1, title: 2001},
      {id: 2, title: 2002},
      {id: 3, title: 2003},
      {id: 4, title: 2004},
      {id: 5, title: 2005},
      {id: 6, title: 2006},
      {id: 7, title: 2007},
      {id: 8, title: 2008},
      {id: 9, title: 2009},
      {id: 10, title: 2010},
      {id: 11, title: 2011},
      {id: 12, title: 2012},
      {id: 13, title: 2013},
      {id: 14, title: 2014},
      {id: 15, title: 2015},
      {id: 16, title: 2016},
      {id: 17, title: 2017},
      {id: 18, title: 2018},
      {id: 19, title: 2019},
      {id: 20, title: 2020},
      {id: 21, title: 2021},
    ]
  };
  const limit = 4;
  const totalPages = Math.ceil(data.list.length / limit);
  const onPageChange = currentPage => {
    // fetch data from server here
    const offset = limit * currentPage;
    renderList(data.list.slice(offset, offset + limit));
    renderPages(currentPage, totalPages);
  };

  paginate({
    container: document.querySelector(".pagination"),
    totalPages,
    pagesToDisplay: 5,
    startPage: 0,
    onPageChange,
    classes: {
      prev: "prev",
      next: "next",
      item: "item-link",
    },
    prevText: "&#8592;",
    nextText: "&#8594;",
  });
})();
* {
  margin: 0;
  padding: 0;
  border: none;
  font-size: 12px;
  list-style: none;
  text-decoration: none;
}

body {
  padding: 20px;
}

.title {
  font-size: 36px;
  color: #f80;
}
.title > small {
  font-size: 24px;
  color: #ccc;
  vertical-align: 4px;
}

.tip {
  color: #999;
}

.emp {
  color: #f80;
}

.list > li {
  padding: 6px;
  border-bottom: 1px solid #eee;
}

.pagination {
  display: inline-block;
  padding: 4px;
}
.pagination a {
  display: inline-block;
  padding: 4px 8px;
  color: #00c;
}
.pagination a.active {
  background: red;
  font-weight: bold;
  color: #f80;
  background-color: #eee;
}
.pagination-tip {
  margin: 0 0 0 10px;
}
<ul class="list"></ul>
<section class="pagination"></section>
<span class="pagination-tip"></span>

\$\endgroup\$

Your Answer

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

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