# Refactoring asynchronous JS pre-rendering code

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) {
.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?

• 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. Jan 7, 2014 at 23:16
• @jfriend00: What is it that you don't understand? I'll happily clarify this.
– Dan
Jan 7, 2014 at 23:18

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 )
{
var promise = model.loadPage( page ).then( function(){
view.renderPage( model.getPage( page ) );
});
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?

• 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.
• 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
Jan 8, 2014 at 10:46
• 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
Jan 8, 2014 at 10:48
• To make it clear: CancellationToken is a class, getNone is its static method. It's not just some global variable.
– Dan
Jan 8, 2014 at 10:54
• Your suggestion might make it easier indeed to review. Jan 8, 2014 at 16:23
• 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!
– Dan
Jan 9, 2014 at 13:24

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