Bug 147667 – Refactor BuildbotQueue.compareIterations and BuildbotQueue.compareIterationsByRevisions to be more generic

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jason Marcell]

BuildbotQueue.compareIterations and BuildbotQueue.compareIterationsByRevisions currently reference the "internal" and "openSource" repositories specifically. These methods should instead work more generically with any number of repositories.

[Jason Marcell]

Created attachment 258244 [details]
Refactor BuildbotQueue.compareIterations and BuildbotQueue.compareIterationsByRevisions to be more generic

[Daniel Bates]

Comment on attachment 258244 [details]
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?

[Jason Marcell]

Comment on attachment 258244 [details]
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
>> +        }
> 
> 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;
> }

I tried to do the caching in the getter itself. Does this not cache the sortedRepositories using an instance variable?

    get sortedRepositories()
    {
        if (this._sortedRepositories === undefined) {
            this._sortedRepositories = this._sortedItems(Dashboard.Repository);
        }
        return this._sortedRepositories;
    },

[Jason Marcell]

(In reply to comment #2)
> > 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?

Okay, but it's not really sorting a dictionary. It's really sorting an array of dictionaries or objects that happen to have an `order` key or property. Maybe `sortArrayByOrder` or `sortDictionariesByOrder` or `sortObjectsByOrder`?

Also, there is a Utilities.js file. Do you think this should go there?

[Jason Marcell]

Created attachment 258530 [details]
Patch

[Daniel Bates]

Comment on attachment 258530 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Utilities.js:201
> +sortDictionariesByOrder = function(unsorted)

Please write this as a named function:

function sortDictionariesByOrder(unsorted)

[Jason Marcell]

Created attachment 258533 [details]
Patch

[Jason Marcell]

Comment on attachment 258533 [details]
Patch

Writing sortDictionariesByOrder as a named function per Dan Bates's suggestion.

[WebKit Commit Bot]

Comment on attachment 258533 [details]
Patch

Clearing flags on attachment: 258533

Committed r188172: <http://trac.webkit.org/changeset/188172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.