Refactoring asynchronous JS pre-rendering code

Question

A few months ago I wrote this module but, coming back to it, I find it a bit hard to read and reason about. I want to ask community's opinion on whether this needs to be refactored, and how I could approach this.

(function (q) {
 'use strict';
 var DELAY_BEFORE_RENDERING_NEXT_PAGE_VIEW = 3 * 1000;
 window.st.viewer.Helper.PageViewFactory = (function () {
 function promiseRenderView(page, token) {
 return page.fetchUnlessReady()
 .then(token.throwIfCanceled)
 .then(function () {
 var view = new window.st.viewer.View.PageView({
 model: page
 });
 view.render();
 return view;
 });
 }
 var scheduledPage,
 scheduledPromise;
 function scheduleRenderNextView(page, token) {
 // After some time passes, prefetch and prerender next page view
 // so if it is requested (very likely), we can re-use an existing promise.
 scheduledPage = null;
 scheduledPromise = null;
 q.delay(DELAY_BEFORE_RENDERING_NEXT_PAGE_VIEW)
 .then(token.throwIfCanceled)
 .then(function () {
 return page.promiseNext();
 })
 .then(token.throwIfCanceled)
 .then(function (nextPage) {
 if (nextPage) {
 scheduledPage = nextPage;
 scheduledPromise = promiseRenderView(nextPage, window.st.Helper.CancellationToken.getNone());
 } else {
 scheduledPage = null;
 scheduledPromise = null;
 }
 })
 .catch(token.catchItself)
 .done();
 }
 function scheduleRenderView(page, token) {
 var promise = (page !== scheduledPage) ?
 promiseRenderView(page, token) :
 scheduledPromise;
 scheduleRenderNextView(page, token);
 return promise;
 }
 return {
 scheduleRenderView: scheduleRenderView
 };
 })();
})(Q);

This module exports a single function called scheduleRenderView. Its purpose is, given a page model and a cancellation token, do the following steps asynchronously:

1. Fully fetch page model (it may be loaded partially);
2. Create and render (in memory) a PageView for it;
3. Return this view to the caller;

4. Additionally, after a 3 second delay:

   • Request next page by calling promiseNext;
   • Pre-render next page by repeating steps 1-3 for it and store the pre-rendered view in case it gets requested the next time scheduleRenderView is called (very likely)

The user views pages one by one, and I wanted to anticipate the most likely scenario where she will request the next page.

I also wanted to make this optimization invisible to the calling code.

Is this code hard to comprehend? Are the simpler ways to achieve the same?

• jfriend00: I must say, this sure looks like a complicated way to express what it is you're trying to do. If it were me, I'd back up to first principles and find a much easier way to express what I was trying to do. I don't understand enough about what you're trying to do to take a crack at it.
• Dan: @jfriend00: What is it that you don't understand? I'll happily clarify this.

Answer 1

Is this code hard to comprehend? Yes.

That is because there are a lot of unknown functions: view.render, page.fetchUnlessReady, token.catchItself ,token.throwIfCanceled or window.st.Helper.CancellationToken.getNone() etc. etc.

In my mind, I would approach this more like :

function scheduleRenderView( page )
{
 //Load and display the page
 var promise = model.loadPage( page ).then( function(){
 view.renderPage( model.getPage( page ) );
 });
 //Load the next page
 model.loadPage( page.next() );
 return promise;
}

loadPage would either return immediately if we retrieved already the data once ( from cache ) or download the page model.

getPage gets a page model from cache

renderPage would do the obvious

This means I would forego the pre-rendering which really sounds like pre-mature optimization, how long could that take?

2 other minor comments:

• Why access some variables through window, that does not seem to make sense
• In my mind, adding functions to the window.st.viewer.Helper is terrible global variable abuse, not to mention that you probably should add it to Helper.prototype.

• Dan: Pre-rendering is not an immature optimization, there was a real need for it. What do you suggest for namespacing instead of window.st.viewer.*? Helper is a namespace, not a class, why add something to its prototype? Again, window.st is just a root namespace.
• Dan: Do you think I should re-word the question to remove any methods specific to my code (fetchUnlessReady, etc, and only leave the "essence" of the code)?
• Dan: To make it clear: CancellationToken is a class, getNone is its static method. It's not just some global variable.
• konijn: Your suggestion might make it easier indeed to review.
• Dan: In fact you were right. Rendering was a premature optimization. It renders view in DOM and begins to fetch images, fonts, etc, so it looks like a useful optimization. What I didn't realize, is that I'm scheduling it too early—before the current view's resources probably have loaded, so it slowed down current view's rendering. I cut this code completely, fixed a couple of problems, and seems to run more smoothly now. So you were right after all!

Answer 2

Two more points to complement konijn's answer to my question:

• I'm moving from window.st.* madness to using require.js with Almond (I wish I did this before);
• Instead of hand-rolled cancellation mechanism, I'm moving from Q to bluebird which already supports opt-in cancellation.