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)); | |
} | |
} | |
); | |
} |
This comment has been minimized.
This comment has been minimized.
<3 Really love this refactor. |
This comment has been minimized.
This comment has been minimized.
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 plz We'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. 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 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 So, we've been focusing on the block of code within that monster 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 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 Consequently, we can simplify the original block of code within that monster 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. |
This comment has been minimized.
This comment has been minimized.
@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 Your condensation of getBandwidth = function( index )
{
var manifestExt = self.manifestExt;
return manifestExt
.getRepresentationFor( index, data )
.then( manifestExt.getBandwidth );
} Notice the slight difference where I directly use the
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 |
This comment has been minimized.
This comment has been minimized.
Thanks to @johnyanarella for his provocative ideas with 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 |
This comment has been minimized.
This comment has been minimized.
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 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
I'm not terribly keen on the closure names here.
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 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. |
This comment has been minimized.
This comment has been minimized.
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
}; |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
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):