-
-
Save ThomasBurleson/7576083 to your computer and use it in GitHub Desktop.
| 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)); | |
| } | |
| } | |
| ); | |
| } |
<3
Really love this refactor.
I'm afraid I have to respectfully disagree.
First, let's acknowledge we're picking nits in a section of code the original author marked with this:
// TODO : I structured this all goofy and messy. fix plzWe've all had deadlines that resulted in code we're not proud of - something to think about before calling people out in the comments of a project announcement article on web development community website and on Twitter. GitHub issues or a pull request might have been a more supportive way to contribute feedback.
With that said:
In your example code, you have extracted and named various micro-operations as new closures within a sprawling function that was already ~175 lines long, expanding a section that was 33 lines of code into 55.
I don't think the answer is introducing more closures.
Like you, when I looked at the original source code, I cringed at the sight of the callback pyramids of doom (isn't this one of the things Promises are supposed to solve?).
Your solution flattens that pyramid, but it does so by utilizing Promises primarily as a queuing mechanism for scheduling sequential function calls. I tend to agree with you that when it comes to Promise callbacks, replacing anonymous closures with named functions tends to improve readability and promote DRY code.
One of the dangerous aspects of your implementation is that your callbacks mutate external state. captureOneDown in particular set off red flags for me. These kinds of side effects are generally verboten in functional programming; in this case, they increase the complexity of the solution (you have to think about the more global aftereffects rather than just thinking locally) and limit the reusability of your named operations in new compositions.
It's important to realize that Promises primarily represent a future value or the transformation of that future value. Yes, in many cases, the final transformation is performing an external operation based on the previous transformations.
Most of the nested operations in the original source code relate to the need to obtain the bandwidth for a given index; the API exposed by manifestExt splits this into two asynchronous operations which need to be called in sequence. Nearly every pyramid in the original source begins with at least one (if not two) of these sequences.
So, my first suggestion would be to add a convenience method to the module that returns the bandwidth for a given index:
getBandwidth: function (index, data) {
var manifestExt = this.manifestExt;
return manifestExt
.getRepresentationFor(index, data)
.then(function (representation) {
return manifestExt.getBandwidth(representation);
});
}This is a typical Promise transformation chain - our new getBandwidth() method returns the future value of a manifest representation transformed into its corresponding bandwidth. Note that there are no external side effects associated with calling this method.
So, we've been focusing on the block of code within that monster checkIndex() function that deals with creating a bitrate downgrade SwitchRequest. So, let's extract that logic into a more manageable module method of its own:
createDowngradeRequest: function (downloadRatio, currentIndex, data) {
var log = this.debug.log;
return Q
.all([
this.getBandwidth(currentIndex - 1, data),
this.getBandwidth(currentIndex, data)
])
.then(Q.spread(function (oneDownBandwidth, currentBandwidth) {
var switchRatio = oneDownBandwidth / currentBandwidth;
log("Switch ratio: ", switchRatio);
if (downloadRatio < switchRatio) {
log("Things must be going pretty bad, switch all the way down.");
return new MediaPlayer.rules.SwitchRequest(0);
}
else {
log("Things could be better, so just switch down one index.");
return new MediaPlayer.rules.SwitchRequest(currentIndex - 1);
}
}));
}Creating a downgrade request involves transforming two bandwidth values into the corresponding SwitchRequest.
One key difference is that we can now leverage Q.all() to execute two calls to our new getBandwidth() module method in parallel, resulting in a future value of those two bandwidth values. We subsequently transform those into the corresponding SwitchRequest. (Q.spread() just allows us to conveniently spread the Array of bandwidth values across function parameters in our callback.) Note that there are again no external side effects associated with calling this method.
And we can do this without the extra convolution of introducing a partial application (clever as those are).
To my eye, the calculations being done are simple enough that I don't see much value in decomposing them further into calculateSwitch and performSwitch operations. We could also quibble about the readability of the if/else conditional branch vs a ternary operator.
Consequently, we can simplify the original block of code within that monster checkIndex() method to:
var log = this.debug.log;
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(this.createDowngradeRequest(downloadRatio, current, data));
}
}Now we have some smaller methods (as small as necessary but no smaller) with no crazy side effects that can be individually tested.
@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.
- I really like your use of
deferred.resolve(this.createDowngradeRequest(downloadRatio, current, data)); - Your use of
Q.all( )andQ.spread( )are great techniques. I will add those to my skill repertoire. ;-) - 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.
The code above is a snapshot of a nice library... but one with deep nestings of promise actions.
I contend that deep nestings are very hard to understand and maintain. And deep nestings often lead to duplicate code blocks. While named closures [used within flattened promise chains] can often be reused.... In effect, flattening promotes DRY code.
Here is a flattening of the above code (lines 1-30 only):