HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Refactoring asynchronous JS pre-rendering code

Submitted by: @import:stackexchange-codereview··
0
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.

(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 a

Solution

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.

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.