Code review comment for lp:~nigelbabu/launchpad/bug-title-849121

Revision history for this message
Gavin Panella (allenap) wrote :

In addition to Raphaël's tip top review, I'd like to add a few points:

[1]

- result['invalid_' + link_type] = invalid_links
+ result[link_type] = invalid_links

invalid_links is not really an appropriate variable name now, because
the value takes the form of {"valid": ..., "invalid": ...}. Something
like link_info would be better I think.

[2]

+ # Remove valid bugs from the list of all the bugs.
+ bugs.remove(task.bug.id)

Can you change bugs to be a set rather than a list? Also, it should
probably be called bug_ids rather than bugs.

[3]

+ for task in bugtasks:
+ valid_links['/bugs/' + str(task.bug.id)] = task.bug.title

It's might just be me, but % formatting would be clearer here:

                valid_links['/bugs/%d' % task.bug.id] = task.bug.title

[4]

+ if( Y.Object.size(invalid_links) === 0 ) {
+ invalid_links = {};
+ }
+ if( Y.Object.size(valid_links) === 0 ) {
+ valid_links = {};
+ }

I didn't know about Object.size(), neat.

It's a nitpick, but if invalid_links is {} already, this will set it
to a new {}. It just doesn't seem very neat ;) How about:

        if (invalid_links === undefined) {
            invalid_links = {};
        }
        if (valid_links === undefined) {
            valid_links = {};
        }

Or use Y.Lang.isValue().

In fact, it's common practice to use implicit truth with JavaScript,
so you can probably combine the above with initialization:

        var invalid_links = link_info[link_type].invalid || {};
        var valid_links = link_info[link_type].valid || {};

[5]

+ if(href in invalid_links) {
...
+ } else if(href in valid_links) {

There were lint complaints about these. For the sake of noise, change
these to use hasOwnProperty().

review: Approve (code)

« Back to merge proposal