Demonstration of refactor of DownloadRatioRules.js > transform deep nesting of promise chains to an easily-maintained, flattened, sequential chain.

DownloadRatioRule.js

if (downloadRatio < 1.0) {
    self.debug.log("Download ratio is poor.");
    if (current > 0) {
        self.debug.log("We are not at the lowest bitrate, so switch down.");
        self.manifestExt.getRepresentationFor(current - 1, data).then(
            function (representation1) {
                self.manifestExt.getBandwidth(representation1).then(
                    function (oneDownBandwidth) {
                        self.manifestExt.getRepresentationFor(current, data).then(
                            function (representation2) {
                                self.manifestExt.getBandwidth(representation2).then(
                                    function (currentBandwidth) {
                                        switchRatio = oneDownBandwidth / currentBandwidth;
                                        self.debug.log("Switch ratio: " + switchRatio);

                                        if (downloadRatio < switchRatio) {
                                            self.debug.log("Things must be going pretty bad, switch all the way down.");
                                            deferred.resolve(new MediaPlayer.rules.SwitchRequest(0));
                                        } else {
                                            self.debug.log("Things could be better, so just switch down one index.");
                                            deferred.resolve(new MediaPlayer.rules.SwitchRequest(current - 1));
                                        }
                                    }
                                );
                            }
                        );
                    }
                );
            }
        );
    } else {
        self.debug.log("We are at the lowest bitrate and cannot switch down, use current.");
        deferred.resolve(new MediaPlayer.rules.SwitchRequest(current));
    }
} else {
    self.debug.log("Download ratio is good.");
    self.manifestExt.getRepresentationCount(data).then(
        function (max) {
            max -= 1; // 0 based
            if (current < max) {
                self.debug.log("We are not at the highest bitrate, so switch up.");
                self.manifestExt.getRepresentationFor(current + 1, data).then(
                    function (representation1) {
                        self.manifestExt.getBandwidth(representation1).then(
                            function (oneUpBandwidth) {
                                self.manifestExt.getRepresentationFor(current, data).then(
                                    function (representation2) {
                                        self.manifestExt.getBandwidth(representation2).then(
                                            function (currentBandwidth) {

                                            }
                                        );
                                    }
                                );
                            }
                        );
                    }
                );
            } else {
                self.debug.log("We are at the highest bitrate and cannot switch up, use current.");
                deferred.resolve(new MediaPlayer.rules.SwitchRequest(max));
            }
        }
    );
}

@johnyanarella In all fairness, I was not nit-picking -> I was offering an opinion and a possible code-refactor improvement to code that was originally reasonable but could be made better by flattening the nestings. And as a global question to all bloggers:"If you blog about your source, would you not want someone to suggest improvements that you may not realize are available ?"

---

Your condensation of getBandwidth() made me laugh. Just this morning, I did the exact same thing:

getBandwidth  = function( index )
{
    var manifestExt = self.manifestExt;
    return manifestExt
                .getRepresentationFor( index, data )
                .then( manifestExt.getBandwidth ); 
}

Notice the slight difference where I directly use the manifestExt.getBandwidth() as resolve handler.

1. I really like your use of deferred.resolve(this.createDowngradeRequest(downloadRatio, current, data));
2. Your use of Q.all( ) and Q.spread( ) are great techniques. I will add those to my skill repertoire. ;-)
3. I also conceded that captureOneDown() is a bit of hackery... but it allows my flattened chains to be a sequence of function references.

if (downloadRatio < 1.0) 
    {
        log("Download ratio is poor.");

        if (current > 0) 
        {
            log("We are not at the lowest bitrate, so switch down.");

            getBandwidth( current - 1 )
               .then( captureOneDown               )
               .then( getBandwidth( current )      )
               .then( calculateSwitchDown          )
               .then( performSwitchDown            );

        } else {

            log("We are at the lowest bitrate and cannot switch down, use current.");
            resolveWith( current );
        }

    } else {

        log("Upgrade possible since download ratio is good.");

        confirmSwitchUp()
            .then( getBandwidth( current + 1 ) )
            .then( captureOneUp                )
            .then( getBandwidth( current )     )
            .then( calculateSwitchUp           )
            .then( performSwitchUp             );
    }

My original refactor code above did not show the full if/else logic of the original. It is the full refactor that demonstrates the value of the chain of promise handlers.

Meanwhile, I need to give your createDowngradeRequest() some more thought as it seems offer a #levelUp over my stack of closures.

Thanks to @johnyanarella for his provocative ideas with Q.all( ) and Q.spread( ). Here is the revised version with an alternate flattening:

var deferrred = Q.defer(),
    log = self.debug.log,
    getBandwidth  = function( index )
    {
        var manifestExt = self.manifestExt;
        log("getBandwidth( "+ index + " )");

        return manifestExt
                    .getRepresentationFor( index, data )
                    .then( manifestExt.getBandwidth ); 
    },
    getMaxBandwidth = function( )
    {
        log("getMaxBandwidth()");

        var manifestExt = self.manifestExt;
        return manifestExt
                .getRepresentationFor( )
                .then( function(last)
                {
                    return max - 1; // zero-index
                };
    },
    calculateUpgrade = function( downloadRatio, current, max, data ) 
    {
        log("calculateUpgrade()");
        var calculateUpgradeIndex = function (ratios) 
            {
                var i, len;
                for ( i = 0, len = ratios.length; i < len; i += 1) 
                {
                    if (downloadRatio < ratios[i]) {
                        break;
                    }
                }
                return i;
            }),
            ratios = [ ];

        while ((i += 1) < max) {
            ratios.push( checkRatio( i, current, data ));
        }

        return  Q.all( ratios )
                 .then( calculateUpgradeIndex )
                 .then( function( index )
                 {
                    log("Calculated ideal upgrade quality index == " + index);
                    return new MediaPlayer.rules.SwitchRequest( index );
                 });
    },
    requestDowngrade = function (downloadRatio, currentIndex, data) 
    {
        log("requestDowngrade()");

        return Q.all([
                    getBandwidth( currentIndex - 1, data ),
                    getBandwidth( currentIndex, data ) 
                ])
                .then( Q.spread(function (previous, current) 
                {
                    var switchRatio = previous / current,
                        message     = (downloadRatio < switchRatio)                               ? 
                                      "Things must be going pretty bad, switch all the way down." :
                                      "Things could be better, so just switch down one index.",
                        newIndex    = (downloadRatio < switchRatio) ? 0 : currentIndex - 1;


                    log( "Switch ratio: ", switchRatio );
                    log( message );

                    return new MediaPlayer.rules.SwitchRequest( newIndex );
                }));
    },
    requestUpgrade = function(downloadRatio, currentIndex, data )
    {
        log("requestUpgrade()");

        return Q.all([
                    getBandwidth( currentIndex, data ),
                    getBandwidth( currentIndex + 1, data ),
                    getMaxBandwidth()
                ])
                .then( Q.spread(function (current, next, max ) 
                {
                    var switchRatio = next / current,
                        newIndex    = downloadRatio < switchRatio ? current     :
                                      downloadRatio > 1000.0      ? max         :
                                      downloadRatio > 100.0       ? next        : -1,

                        message     = downloadRatio < switchRatio ? "Not enough bandwidth to switch up."              :
                                      downloadRatio > 1000.0      ? "Tons of bandwidth available, go all the way up." :
                                      downloadRatio > 100.0       ? "Just enough bandwidth available, switch up one." : 
                                                                    "Not exactly sure where to go, so do some math.";
                    log( "Switch ratio: ", switchRatio );
                    log( message );

                    return  (newIndex < 0) ? 
                            calculateUpgrade( downloadRatio, current, max, data ) :
                            new MediaPlayer.rules.SwitchRequest( newIndex );
                }));
    };

// *******************************************************
// Deep-nestings transformed to requests with grouping and chaining...
// *******************************************************

if (downloadRatio < 1.0) 
{
    log("Download ratio is poor.");
    if (current > 0) 
    {
        log("We are not at the lowest bitrate, so switch down.");
        deferred.resolve( 
            requestDowngrade( downloadRatio, current, data ) 
        );

    } else {

        log("We are at the lowest bitrate and cannot switch down, use current.");
        deferred.resolve( current );
    }

} else {

    log("Upgrade possible since download ratio is good.");
    deferred.resolve( 
        requestUpgrade( downloadRatio, current, data ) 
    );
}

return deferred.promise;

Obviously the secret to this solution is deferred.resolve( ) resolving itself with a promise [chain]. Nice.

I value the many long conversations I've had with you where we've whittled away at a problem to find the most elegant solution, and there is some pretty concrete evidence above that I agree with the value of suggesting improvements. My concern just relates to the venue and manner in which one offers unsolicited advice and how that might be received.

Digital Primates is in the midst of a high profile promotional campaign for this library (which they developed in collaboration with Microsoft) that includes that blog article and a presentation in partnership with Microsoft, Google and Akamai at Streaming Media West last week. I'm excited to see people we know playing a big role in moving the web forward.

Since we know Jeff and many of the Digital Primates crew, and are used to sharing ideas amongst our tribe of Adobe Flex refugees, it's easy to miss the context here. The fact is, the last thing they need in the midst of a project launch is a bunch of their colleagues initiating an unsolicited critique of a subset of their code in the midst of their virtual press conference moment.

I wanted to address the concern that this discussion (and the manner in which it originated) might be misconstrued as a kind of "me, too" brand of self-promotion. I didn't want to contribute in publicly piling on in a critique of their code without at least acknowledging these concerns and clarifying intent.

I think I speak for both of us when I say, (in case there was any doubt) we know full well how much effort they must have put into dash.js. We share many of the same battle scars, so we're fans of their team and want them to be successful.

---

Returning to the topic of discussing ways we would hone this code into a polished gem:

Be careful - methods aren't typically bound to their instance scope in JavaScript.

When passing manifestExt.getBandwidth() to then(), you are really passing a reference to the getBandwidth() function which will be run in the global object scope (ex. window). That might be fine for a static function, but will quickly backfire if you call a method function that relies on instance state.

Where possible, your version does read better. This reminds me of a discussion I had earlier this year with the SpatialKey guys where a couple of them were advocating the value of including automatic instance scope binding for methods as part of a class / module factory helper; that approach eliminates an entire class of errors (no pun intended).

I like that you expanded the scope of your example to include those other conditions (upgrade, downgrade, etc.) - your new example really demonstrates how much all of these suggestions trim down the original checkIndex() method and clarify its operation.

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

I'm not terribly keen on the closure names here.

calculateUpgrade() implies it returns a calculation, not a SwitchRequest.

requestUpgrade() and requestDowngrade() imply they perform the action of requesting an upgrade or downgrade, but in fact return a SwitchRequest.

I still think these should be promoted from inline closures to (internal) methods of the module. There's no necessity to recreate these on every execution of the checkIndex method, and there are only downsides in doing so (increased memory usage, more work for the garbage collector, slower performance, etc.).

I also think the ternary operator use is not promoting clarity. Three nested ternary expressions are a pretty good indication that the calculation needed to initialize a variable is complex enough it should probably be extracted into its own function. Repeating the same nested ternary expression twice in immediate succession means that the same expressions have to be unnecessarily reevaluated, and makes it far less readable and obvious that they're both implementations of the same conditional branches.

I think by the end of this we're going to have some great material for a pull request.

Getting close:

MediaPlayer.rules.DownloadRatioRule = function () {
"use strict";

    var self = null,
        noop = function() { },
        $log = null,                // updated during self.checkIndex()
        getRepresentation = null,   // updated during self.checkIndex()
        getBandwidth      = null,   // updated during self.checkIndex()

        /**
         *
         */
        getIndexedBandwidth  = function( index, data )
        {
            $log("getIndexedBandwidth( "+ index + " )");
            return getRepresentation( index, data ).then( getBandwidth ); 
        },
        /**
         *
         */
        getMaxBandwidth = function( )
        {
            $log("getMaxBandwidth()");

            return getRepresentation( )
                        .then( function(last)
                        {
                            return max - 1; // zero-index
                        };
        },
        /**
         *
         */
        checkRatio = function (newIdx, currentBandwidth, data) 
        {  
            $log("checkRatio()");

            return getIndexedBandwidth( newIdx, data)
                        .then( newBandwidth )
                        {
                            return (newBandwidth / currentBandwidth);
                        });
        },
        /**
         *
         */
        calculateTimesAndRatios = function( metrics )
        {
            var DOWNLOAD_RATIO_SAFETY_FACTOR = 0.75,
                httpRequests = metrics.HttpList,
                numRequests  = httpRequests ? httpRequests.length : 0,
                lastRequest  = numRequests  ? httpRequests[numRequests - 1] : null,
                info = {
                    totalTime     : lastRequest ? (lastRequest.tfinish.getTime() - lastRequest.trequest.getTime()) / 1000   : 0,
                    downloadTime  : lastRequest ? (lastRequest.tfinish.getTime() - lastRequest.tresponse.getTime()) / 1000  : 0,
                    totalRatio    : lastRequest ? lastRequest.mediaduration / totalTime                                     : NaN,
                    downloadRatio : lastRequest ? (lastRequest.mediaduration / downloadTime) * DOWNLOAD_RATIO_SAFETY_FACTOR : NaN
                };

            $log("calculateTimesAndRatios() - Checking download ratio rule...");

            if (!metrics) {

                $log("No metrics, bailing.");
                info = null;

            } else if ( numRequests === 0) {

                $log("No requests made for this stream yet, bailing.");
                info = null;

            } else if (info.totalTime <= 0) {

                $log("Don't know how long the download of the last fragment took, bailing.");
                info = null;

            } else if ( !lastRequest.mediaduration || lastRequest.mediaduration <= 0) {

                $log("Don't know the duration of the last media fragment, bailing.");
                info = null;

            } else if ( isNaN(downloadRatio) || isNaN(totalRatio) ) {

                $log("Total time: " + totalTime + "s");
                $log("Download time: " + downloadTime + "s");
                $log("The ratios are NaN, bailing.");

                info = null;
            }

            $log("Total ratio: "    + totalRatio);
            log("Download ratio: " + downloadRatio);

            return info;
        },
        /**
         *
         */
        calculateUpgradeSwitch = function( downloadRatio, current, max, data ) 
        {
            $log("calculateUpgrade()");

            var calculateUpgradeIndex = function (ratios) 
                {
                    var i, len;
                    for ( i = 0, len = ratios.length; i < len; i += 1) 
                    {
                        if (downloadRatio < ratios[i]) {
                            break;
                        }
                    }
                    return i;
                }),
                ratios = [ ];

            while ((i += 1) < max) {
                ratios.push( checkRatio( i, current, data ));
            }

            return  Q.all( ratios )
                     .then( calculateUpgradeIndex )
                     .then( function( index )
                     {
                        $log("Calculated ideal upgrade quality index == " + index);
                        return new MediaPlayer.rules.SwitchRequest( index );
                     });
        },
        /**
         *
         */
        requestDowngradeSwitch = function (downloadRatio, currentIndex, data) 
        {
            $log("requestDowngrade()");

            return Q.all([
                        getIndexedBandwidth( currentIndex - 1, data ),
                        getIndexedBandwidth( currentIndex, data ) 
                    ])
                    .then( Q.spread(function (previous, current) 
                    {
                        var switchRatio = previous / current,
                            message     = (downloadRatio < switchRatio)                               ? 
                                          "Things must be going pretty bad, switch all the way down." :
                                          "Things could be better, so just switch down one index.",
                            newIndex    = (downloadRatio < switchRatio) ? 0 : currentIndex - 1;


                        $log( "Switch ratio: ", switchRatio );
                        $log( message );

                        return new MediaPlayer.rules.SwitchRequest( newIndex );
                    }));
        },
        /**
         *
         */
        requestUpgradeSwitch = function(downloadRatio, currentIndex, data )
        {
            $log("requestUpgrade()");

            return Q.all([
                        getIndexedBandwidth( currentIndex, data ),
                        getIndexedBandwidth( currentIndex + 1, data ),
                        getMaxBandwidth()
                    ])
                    .then( Q.spread(function (current, next, max ) 
                    {
                        var switchRatio = next / current,
                            newIndex    = downloadRatio < switchRatio ? current     :
                                          downloadRatio > 1000.0      ? max         :
                                          downloadRatio > 100.0       ? next        : -1,

                            message     = downloadRatio < switchRatio ? "Not enough bandwidth to switch up."              :
                                          downloadRatio > 1000.0      ? "Tons of bandwidth available, go all the way up." :
                                          downloadRatio > 100.0       ? "Just enough bandwidth available, switch up one." : 
                                                                        "Not exactly sure where to go, so do some math.";
                        $log( "Switch ratio: ", switchRatio );
                        $log( message );

                        return  (newIndex < 0) ? 
                                calculateUpgradeSwitch( downloadRatio, current, max, data ) :
                                new MediaPlayer.rules.SwitchRequest( newIndex );
                    }));
        };

        // ******************************************
        // Publis DownloadRatioRule instance and API 
        // ******************************************

        return self = {
            /**
             * Post-construction injection to logger reference
             */ 
            debug       : undefined,
            /**
             * Post-construction injection to Manifest model
             */
            manifestExt : undefined,
            /**
             * Check QoS to determine if upgrade or downgrade is warranted...  
             */
            checkIndex  : function (current, metrics, data)
            {
                var deferrred = Q.defer(),
                    info      = null;

                // Dynamic update specific closure variables
                $log              = ( self.debug && self.debug.log ) ? self.debug.log : noop;
                getRepresentation = _.bind( self.manifestExt.getRepresentationFor, self.manifestExt ),
                getBandwidth      = _.bind( self.manifestExt.getBandwidth,         self.manifestExt ),


                $log( "DownloadRatioRule::checkIndex()" );

                info = calculateTimesAndRatios( metrics );

                if ( !info )
                {
                    deferred.resolve ( 
                        // @TODO - is this correct ?
                        new MediaPlayer.rules.SwitchRequest( );
                    );

                } else {

                    if (info.downloadRatio < 1.0) 
                    {
                        $log("Download ratio is poor.");
                        if (current > 0) 
                        {
                            $log("We are not at the lowest bitrate, so switch down.");
                            deferred.resolve( 
                                requestDowngradeSwitch( info.downloadRatio, current, data ) 
                            );

                        } else {

                            $log("We are at the lowest bitrate and cannot switch down, use current.");
                            deferred.resolve( 
                                new MediaPlayer.rules.SwitchRequest( current ) 
                            );
                        }

                    } else {

                        $log("Upgrade possible since download ratio is good.");
                        deferred.resolve( 
                            requestUpgradeSwitch( info.downloadRatio, current, data ) 
                        );
                    }
                }

                return deferred.promise;
            }
        };
};

MediaPlayer.rules.DownloadRatioRule.prototype = {
    constructor: MediaPlayer.rules.DownloadRatioRule
};

Thanks for this example, Thomas. Keep up the good work, I don't think any of this was disrespectful to the people behind Dash.js or could be misconstrued as such -- I'm not sure John was a neutral party in this discussion, but his input certainly got the ball rolling.