patternjavascriptMinor
Refactoring asynchronous JS pre-rendering code
Viewed 0 times
refactoringasynchronousprecoderendering
Problem
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.
This module exports a single function called
(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 aSolution
Is this code hard to comprehend? Yes.
That is because there are a lot of unknown functions:
In my mind, I would approach this more like :
This means I would forego the pre-rendering which really sounds like pre-mature optimization, how long could that take?
2 other minor comments:
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 cacherenderPage would do the obviousThis 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.Helperis terrible global variable abuse, not to mention that you probably should add it toHelper.prototype.
Code Snippets
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;
}Context
StackExchange Code Review Q#38800, answer score: 2
Revisions (0)
No revisions yet.