Optimize KnockoutJs Pagination Computed Function

Question

I have written a function that builds an array of paging options for rendering on the view.

I have modelled it after DataTables for jQuery, a live example with lots of data is available here: http://www.companieshousedata.co.uk/a/490

Is there a better way to write this? The function also returns ellipsis (as strings) where necessary.

Model/object:

function PageDirection(data) {
 var self = this;
 self.display = data.display;
 self.value = data.value; 
 self.disabled = function (currentPage, lastPage) {
 return self.value() > lastPage || self.value() < 1 || self.value() == null;
 };
}

Computed observable:

self.pageDirections = ko.computed(function () { 
 var pageIndex = self.pageIndex();
 var lastPage = self.lastPage();
 //Previous
 var directions = [
 new PageDirection({
 display: "Previous",
 value: ko.computed(function() {
 return parseInt(pageIndex, 10) - 1;
 })
 })
 ];
 //6 or less pages
 if (self.lastPage() <= 6) {
 for (var i = 0; i < lastPage; i++) {
 directions.push(new PageDirection({
 display: i + 1,
 value: ko.observable(i + 1)
 }));
 }
 } else {
 if (pageIndex > 4) {
 //1
 directions.push(new PageDirection({
 display: 1,
 value: ko.observable(1)
 }));
 //...
 directions.push(new PageDirection({
 display: "...",
 value: ko.observable(null)
 }));
 if (lastPage - pageIndex <= 3) {
 //last 5
 for (var k = lastPage - 5; k < lastPage; k++) {
 directions.push(new PageDirection({
 display: k + 1,
 value: ko.observable(k + 1)
 }));
 } 
 } else {
 //-1
 directions.push(new PageDirection({
 display: pageIndex - 1,
 value: ko.observable(pageIndex - 1)
 }));
 //current
 directions.push(new PageDirection({
 display: pageIndex,
 value: ko.observable(pageIndex)
 }));
 }
 if (lastPage - pageIndex > 3) {
 //+1
 directions.push(new PageDirection({
 display: pageIndex + 1,
 value: ko.observable(pageIndex + 1)
 }));
 //...
 directions.push(new PageDirection({
 display: "...",
 value: ko.observable(null)
 }));
 }
 } else {
 //first 5
 for (var l = 0; l < 5 ; l++) {
 directions.push(new PageDirection({
 display: l + 1,
 value: ko.observable(l + 1)
 }));
 }
 //...
 directions.push(new PageDirection({
 display: "...",
 value: ko.observable(null)
 }));
 }
 if (lastPage - pageIndex > 3) {
 //last
 directions.push(new PageDirection({
 display: lastPage,
 value: ko.observable(lastPage)
 }));
 } 
 }
 //Next
 directions.push(new PageDirection({
 display: "Next",
 value: ko.computed(function () {
 return parseInt(pageIndex, 10) + 1;
 })
 }));
 return directions;
});

View (for reference):

<span data-bind="foreach: pageDirections">
 <span data-bind="text: display, click: $parent.navigate.bind($parent), css: { 'inactive': disabled($parent.pageIndex(), $parent.lastPage()), 'active': $parent.pageIndex() == value() }"></span>
</span>

Answer 1

Let's take a look at that first line of the PageDirection function:

var self = this;

This is a common practice to keep a reference to the this object within the closure. A different way to do this would be to bind the context of the closure using Function.bind()

this.disabled = function (currentPage, lastPage) {
 return this.value() > lastPage || this.value() < 1 || this.value() == null;
}.bind(this);

However, there appears to be no need to bind the context

function PageDirection(data) {
 this.display = data.display;
 this.value = data.value; 
 this.disabled = function (currentPage, lastPage) {
 console.log('this.value: ', this.value(), ' > lastPage: ', this.value() > lastPage);
 return this.value() > lastPage || this.value() < 1 || this.value() == null;
 };
}
const pd1 = new PageDirection({value: _ => 16, display: 'BMW'});
console.log('pd1 disabled: ', pd1.disabled(1, 10));

Though maybe the KO code invokes that method differently, in which case the function may need to be bound.

Another option would be to use an ES6 arrow function expression, which doesn't have its own this binding:

this.disabled = (currentPage, lastPage) => 
 this.value() > lastPage || this.value() < 1 || this.value() == null;

---

To be honest, the logic in the observable looks a bit overly-complex. I didn't fully study it and grok it but I would look to see if some of the logic can be simplified - for example:

} else {
 if (pageIndex > 4) {

Unless it is the case that the subsequent logic wouldn't work you might consider if that can be converted to:

} else if (pageIndex > 4) {

Which could decrease the sub-sequent indentation levels.