[Webkit-unassigned] [Bug 147667] Refactor BuildbotQueue.compareIterations and BuildbotQueue.compareIterationsByRevisions to be more generic

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 7 13:31:49 PDT 2015


https://bugs.webkit.org/show_bug.cgi?id=147667

--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 258244
  --> https://bugs.webkit.org/attachment.cgi?id=258244
Refactor BuildbotQueue.compareIterations and BuildbotQueue.compareIterationsByRevisions to be more generic

View in context: https://bugs.webkit.org/attachment.cgi?id=258244&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:276
> +        for (var i = 0; i < Dashboard.sortedRepositories.length; i++) {
> +            var repositoryName = Dashboard.sortedRepositories[i].name;
> +            var result = b.revision[repositoryName] - a.revision[repositoryName];
> +            if (result)
> +                return result;
> +        }

Notice that the getter Dashboard.sortedRepositories is called twice on each iteration: once in the for-loop condition "i < Dashboard.sortedRepositories.length" (line 271) and once when evaluating "Dashboard.sortedRepositories[i].name" (line 272). We should cache Dashboard.sortedRepositories and Dashboard.sortedRepositories.length in local variables to avoid extraneous calls to Dashboard.sortedRepositories, such that this for-loop reads:

var sortedRepositories = Dashboard.sortedRepositories;
for (var i = 0, length = sortedRepositories.length; i < sortedRepositories.length; ++i) {
    var repositoryName = sortedRepositories[i].name;
    var result = b.revision[repositoryName] - a.revision[repositoryName];
    if (result)
        return result;
}

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:293
> +        for (var i = 0; i < Dashboard.sortedRepositories.length; i++) {
> +            var repositoryName = Dashboard.sortedRepositories[i].name;
> +            var result = b.revision[repositoryName] - a.revision[repositoryName];
> +            if (result)
> +                return result;
> +        }

Similarly, we should cache Dashboard.sortedRepositories and Dashboard.sortedRepositories.length in local variables.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Dashboard.js:45
> +        if (this._sortedPlatforms === undefined) {

Remove the brace for if-statements with a single line body.

Also, I would write this line as:

if (!this._sortedPlatforms)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Dashboard.js:52
> +        if (this._sortedRepositories === undefined) {

Remove the brace for if-statements with a single line body.

Similar to above, I would write this line as:

if (!this._sortedRepositories)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Dashboard.js:68
> +    _sortedItems: function(unsorted)
> +    {
> +        var sorted = [];
> +
> +        for (var key in unsorted)
> +            sorted.push(unsorted[key]);
> +
> +        sorted.sort(function(a, b) {
> +            return a.order - b.order;
> +        });
> +
> +        return sorted;

Please make this a free function. Additionally, the name of this function is disingenuous given the presence of its argument. Specifically the use of the past tense word "sorted" implies the function is a getter and/or sorts some kind of global data "items". But it takes an unsorted array as its argument and sorts this argument. Functions that perform a computation should have a name that begins with a verb. Maybe sortDictionaryByOrder()? Can we come up with a better name?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150807/2aaae577/attachment.html>


More information about the webkit-unassigned mailing list