-
-
Save zspencer/2979078 to your computer and use it in GitHub Desktop.
/* | |
This first iteration only paginated an initial set of pages. Adding or removing pages without | |
reinitializing the plugin would stay with the initial set. | |
Everything was encapsulated within a single jQuery plugin which knew everything about pagination: | |
* buttons/etc for moving around pages | |
* jumping to a page | |
* animating moving to a new page | |
* where in the dom the page numbers were stored | |
* updating the ui based upon which page you scrolled to | |
*/ | |
$.fn.extend({ | |
paginate: function() { | |
var $self = $(this); | |
var currentPage; | |
var pages = function() { | |
return $self.find('.page'); | |
} | |
var init = function() { | |
currentPage = pages().first().data('page'); | |
pages().waypoint(function() { | |
currentPage = $(this).data('page'); | |
$('#jump-pages').val(currentPage); | |
}); | |
$("#jump-pages").html(''); | |
_.each(pages(), function(page) { | |
var p = $(page).data('page'); | |
$('#jump-pages').append('<option value="' + p + '">' + p + '</option>'); | |
}); | |
} | |
var scrollToPage = function(pageCount) { | |
var ps = _.map(pages(), function(p) { return $(p).data('page') }); | |
currentPage = ps[_.indexOf(ps, currentPage) + pageCount] || currentPage; | |
var offset = pageCount < 0 ? -10 : 10; | |
$('html, body').animate({ scrollTop: $('.page[data-page="'+ currentPage + '"]').offset().top + offset }); | |
} | |
$('#next-page').on('click', function() { | |
scrollToPage(+1); | |
}); | |
$('#jump-pages').on('change', function() { | |
scrollToPage($(this).val() - currentPage); | |
}); | |
$('#previous-page').on('click', function() { | |
scrollToPage(-1); | |
}); | |
init(); | |
return { refresh: init } | |
} | |
}); |
$.fn.extend({ | |
rendersPages: function(pages) { | |
var paginator = Paginator({ | |
pageNumber: $('#jump-pages'), | |
previous: $('#previous-page'), | |
next: $('#next-page'), | |
}, $(this)); | |
var pageTemplate = { | |
page: _.template($('#page-template').html()) | |
} | |
pages.bind('pageLoaded', function(p) { | |
var newPage = $(pageTemplate.page(p)); | |
$this.append(newPage); | |
paginator.paginate(); | |
}); | |
} | |
}); | |
(function() { | |
window.Paginator = function(controls, $pagesContainer) { | |
var pages = function() { | |
return $pagesContainer.find('.page'); | |
} | |
var currentPage = pages().first().data('page'); | |
controls.next.on('click', function() { | |
scrollToPage(+1); | |
}); | |
controls.pageNumber.on('change', function() { | |
scrollToPage($(this).val() - currentPage); | |
}); | |
controls.previous.on('click', function() { | |
scrollToPage(-1); | |
}); | |
var paginate = function() { | |
pages().waypoint(function() { | |
currentPage = $(this).data('page'); | |
controls.pageNumber.val(currentPage); | |
}); | |
controls.pageNumber.html(''); | |
_.each(pages(), function(page) { | |
var p = $(page).data('page'); | |
controls.pageNumber.append('<option value="' + p + '">' + p + '</option>'); | |
}); | |
} | |
var scrollToPage = function(pageCount) { | |
var ps = _.map(pages(), function(p) { return $(p).data('page') }); | |
var nextPage = ps[_.indexOf(ps, currentPage) + pageCount] || currentPage; | |
$('html, body').animate({ scrollTop: $('.page[data-page="'+nextPage + '"]').offset().top + pageCount }); | |
} | |
return { paginate: paginate } | |
} | |
})(); |
$.fn.extend({ | |
rendersPages: function(pages) { | |
$pages = $(this); | |
var paginator = $pages.paginate(); | |
pages.bind('pageLoaded', function(p) { | |
var newPage = $(pageTemplate.page(p)); | |
$pages.append(newPage); | |
paginator.refresh(); | |
}); | |
} | |
}); | |
$.fn.extend({ | |
paginate: function() { | |
var $pages = (function($this) { return function() { | |
return $this.find('.page'); | |
} })($(this)) | |
var convertDomPagesToArray = function() { | |
return _.map($pages(), function(p) { return '' + $(p).data('page'); }); | |
} | |
var paginator = Paginator($('#jump-pages').val(), convertDomPagesToArray()); | |
paginator.refresh = function() { | |
paginator.paginate($('#jump-pages').val(), convertDomPagesToArray()); | |
$("#jump-pages").html(''); | |
_.each(pages, function(p) { | |
$('#jump-pages').append('<option value="' + p + '">' + p + '</option>'); | |
}); | |
$pages().waypoint(function() { | |
var currentPage = $(this).data('page'); | |
paginator.updateCurrentPage(currentPage); | |
$('#jump-pages').val(currentPage) | |
}); | |
} | |
$('#previous-page').on('click', paginator.previous); | |
$('#next-page').on('click', paginator.next); | |
$('#jump-pages').on('change', function() { | |
paginator.jumpTo($(this).val()); | |
}); | |
paginator.on('pageChanged', function(p, difference) { | |
var offset = difference < 0 ? -1 : 1 | |
$('html, body').animate({ scrollTop: $('#page-' + p).offset().top + offset}); | |
}) | |
return paginator; | |
} | |
}); | |
(function(exports) { | |
exports.Paginator = function(currentPage, pages) { | |
var paginator = {}; | |
asEvented.call(paginator); | |
paginator.previous = function() { | |
scrollToPage(-1); | |
}; | |
paginator.next = function() { | |
scrollToPage(+1); | |
} | |
paginator.jumpTo = function(newPage) { | |
scrollToPage(_.indexOf(pages, newPage) - _.indexOf(pages, currentPage)); | |
} | |
var paginate = paginator.paginate = function(newCurrentPage, newPageSet) { | |
pages = newPageSet; | |
updateCurrentPage(newCurrentPage); | |
} | |
var updateCurrentPage = paginator.updateCurrentPage = function(newCurrentPage) { | |
currentPage = newCurrentPage; | |
} | |
var scrollToPage = function(pageCount) { | |
updateCurrentPage(pages[_.indexOf(pages, currentPage) + pageCount] || currentPage); | |
paginator.trigger('pageChanged', currentPage, pageCount); | |
} | |
return paginator | |
} | |
})(window); |
Great to see your three steps to reach where you ended up. But take my comments with a grain of salt because I'm sure there are more concerns going with the rest of the codebase that I don't understand.
Why do you need a "Paginator"? Are you planning to use a pagination concept outside of this jQuery plugin? I only ask because separating responsibilities doesn't always mean "add a new class". Your first pass looked quite separated already. Every event handler only did enough that it had to worry about. All state to handle pagination is self contained within that closure. Your concept of DOM pagination seemed pretty straightforward. I share your suspicion that the extra code by the time you reach step three isn't helping.
I think separating concerns is a good thing. But I also think that the 30 added lines are a bit crazy.
Some speculations why that happened:
If you are building something on top of something else that requires you to write your code in a particular way, it might cost a lot to write your code in a different style. Perhaps it is awkward to write testable jquery plugins?
Your way of reaching the separation might not be the only one. There might be a different way to separate concerns without adding those 30 extra lines. I've not studied your code thoroughly, but one thing you can try is to remove the responsibility of actually switching page from the paginator. Just have the paginator be a data structure which you can query for the current page and then change the page. Then the jquery plugin can initialize it with the pages and then on the click events you can do pager.next(); showPage(pager.getCurrentPageId());.
Another vague feeling I have is that you can tear things apart and glue things together in a different way in functional languages. It might be useful to learn a functional language to learn about a different way to structure a program. Some ways might work and be useful in OOP.
Did that help at all?
You have created a lot of interfaces in your final version which were not in your original version. Are they useful to you?
As a general rule, generality has costs -- you give up special case efficiency for the ability to deal with additional cases. This can be good when those are useful cases. This can be bad when those cases have nothing to do with the problems you need to solve. (Simplicity is a virtue.)
Regardless of what DHH says about the matter, separating concerns is clearly a good idea. After looking through these examples, though, I was feeling uneasy about the refactoring, even though conceptually I agreed with what you were doing. I think what is giving me a hard time is that this is more than a refactoring, it also adds new functionality. The idea of pages being added on the fly and "refreshing" the pagination and the "pageLoad" event does not exist in the first example, so the "refactored" version is more complex than the original because of that. As a rule, I try to either add functionality or refactor code without mixing them so that I can compare apples to apples.
I would be interested to see what the refactored code would look like with this approach without the additional functionality.
@chrisjpowers: I've updated the first step with a version that is functionally equivalent to later sections.
When scanning the first example, it's easier for me to devise what's going on. Also, I like the idea of having all the knowledge required to paginate encapsulated in the single plugin. That, in itself, is a form of separating responsibility by keeping that knowledge out of the presentation layer.
On the last example, the first thought I had was; Why would you need to reuse the Paginator object itself outside the context of the plugin? You've named the dom attributes required to bootstrap the paginator generic enough that the plugin itself is quite reusable. I know now why you did it (SoC), but I don't think it's worth the extra code and I think exposing another interface is confusing.
I'm tending to agree with your final assertion.
My primary objective for this refactor was to have 2 objects: 1 which knew about the DOM, the UI Events, the API events, etc. and one which knew about the mechanics of paginating (keeping track fo which page it is on, etc).
Though I've reached this objective and made the Paginator object pretty reusable, I'm miffed that it came at the cost of about 30 lines of code.
I've added a couple features during the refactor and fixed a couple of bugs; but at the same time I still have the nagging question:
"Is separating responsibilities necessarily worth it if the responsibility is well encapsulated behind a very small interface?"
In this case, I'm leaning towards no. Yes the first pass was very dense; but it's at least relatively readable.
Thoughts?