4
\$\begingroup\$

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?

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

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

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

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.