4
\$\begingroup\$

I wanted to copy the behavior of github's pagination. Here is what it turned out: http://jsbin.com/dihohiseca/5/edit?js,output

It works fine, but I have a feeling that there is a lot of room for improvements. Please help me make my code more elegant / shorter.

var page = 12;
var numPages = 40;
var res = [];

var from = 1;
var to = numPages;
if (numPages > 10) {
  from = Math.max(page - 2, 1);
  to = Math.max(Math.min(page + 2, numPages), 5);
  if (to > 5) {
    res.push(1);
    if (from > 2) res.push(2);
    if (from > 3) {
      if (from === 4) {
        res.push(3);
      } else {
        res.push("...");
      }
    }
  }
}

for (var i = from; i <= to; i++) {

  res.push(i);

}

if (numPages > 10) {
  if (to < (numPages - 2)) {
    if (to === 8) {
      res.push(9);
    } else {
      res.push("...");
    }
  }
  if (to < numPages)
    res.push(numPages - 1);
  if (to !== numPages)
    res.push(numPages);
}

console.log(res);
$("body").html(res.toString());
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.0/jquery.min.js"></script>

The part with bigger page number was easy,

var page = 7;
var numPages = 155;
// [1, 2, "...", 5, 6, 7, 8, 9, "...", 154, 155]
//https://github.com/emberjs/ember.js/issues?page=7&q=is%3Aissue+is%3Aclosed

but with page number 11 the code got very messy.

var page = 5;
var numPages = 11;
// [1, 2, 3, 4, 5, 6, 7, "...", 10, 11]
// https://github.com/emberjs/ember.js/issues?page=7&q=is%3Aissue+is%3Aopen


var page = 6;
var numPages = 11;
// [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]
// https://github.com/emberjs/ember.js/issues?page=6&q=is%3Aissue+is%3Aopen
\$\endgroup\$
1
\$\begingroup\$

I assume that page and numPages are the only variables you actually get, and the code is a snippet inside some function (if not, put it in one!). So first is... your code is pretty much imperative. Nothing wrong with it but the usual "issues" I see is some really nested if statements and a bunch of variables for tracking purposes. We'll take care of that in a bit.

Next would be variable naming. page doesn't tell me much. Is it the current page? I think so. So name it currentPage instead. Same with the other variables. Name them to describe what they're supposed to do. numPages? How about totalPages? res... what's that?

Anyways, here's my take on your code in a less if-else-ish manner. It isolates the logic down to the very last moment, in the ternary. If I placed a breakpoint just before that, I can easily see my variables and infer from them what happens next. Compare that to a deep if-else code where you have to step through the code in order to know how it ran, which branch it took this time etc.

function range(n){
  return Array.apply(null, Array(n)).map(function (_, i) {return i;});
}

function generatePagination(currentPage, totalPages){

  // First we move out the configurations. That way, they don't mingle with the logic.
  var initialChunkPadding = 1;
  var middleChunkPadding = 2;
  var endingChunkPadding = 1;
  var gapValue = '...';

  // Instead of a loop, we use range. It's much cleaner and we don't have tracking variables
  // at the cost of generating an array.
  return range(totalPages).reduce(function(pagination, index){

    // Then we determine what the current page is based on some comparisons
    var page = index + 1;
    var isInitialChunk = page <= 1 + initialChunkPadding;
    var isMiddleChunk = page >= currentPage - middleChunkPadding && page <= currentPage + middleChunkPadding;
    var isEndingChunk = page >= totalPages - endingChunkPadding;
    var hasNoGap = pagination[pagination.length - 1] !== gapValue;

    // Then based on the determinations, we determine what value gets pushed into the array.
    // It can either be the page, a '...', or a blank array (which doesn't change anything with concat)
    var valueToAdd = isInitialChunk || isMiddleChunk || isEndingChunk ? page : hasNoGap ? gapValue : [];
    
    return pagination.concat(valueToAdd);
  }, []);
}

// StackExchange should seriously add a console view >.<
document.write(JSON.stringify(generatePagination(16, 40)));
document.write('<br>');
document.write(JSON.stringify(generatePagination(7, 155)));
document.write('<br>');
document.write(JSON.stringify(generatePagination(5, 11)));
document.write('<br>');
document.write(JSON.stringify(generatePagination(6, 11)));

\$\endgroup\$
  • \$\begingroup\$ excellent solution and explanation, but I found a tiny mistake. generatePagination(1, 21) results: [1,2,3,"...",20,21], instead of [1,2,3,4,5,"...",20,21] . \$\endgroup\$ – user3568719 Oct 24 '15 at 15:05
  • 1
    \$\begingroup\$ And generatePagination(20, 21) results [1,2,"...",19,20,21] which should be [1,2,"...", 17, 18,19,20,21] see :github.com/strongloop/loopback/… (this is a bug in my code too) \$\endgroup\$ – user3568719 Oct 24 '15 at 15:28
  • \$\begingroup\$ @user3568719 Ahh, yep. That's a bug. The nice thing though is that the reduce is running through all page numbers. You can simply add a condition (I guess 2, one for the starting 5, and ending 5), and include them in the ternary that adds the page. \$\endgroup\$ – Joseph Oct 24 '15 at 17:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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