Issue 5177

[Michael Gundlach]

Pao, you asked me to give feedback on r5158 on Issue 5177.  To avoid cluttering the Issue comments, and to let anyone else who's interested hear or comment, I'm starting this thread.

Structure

I like the organization a lot :)  Thanks!  It's so nice to start at the bottom of the .js file with a couple simple method calls, then drill into various objects.

There are two main ways AdBlock's code tends to define objects: the "prototype" way and the "closure" way.  You are using the closure approach, where you have truly private methods that are inaccessible from outside the object.  The prototype way uses "fake private" methods prepended with underscores:

function Foo() { };

Foo.prototype = {

  _somePrivateMethod: function() { ... },

  somePublicMethod: function() { ... this._somePrivateMethod(); ...}

};

foo = new Foo();

A downside to the "closure" way is that you cannot write unit tests against the private methods.  Now that AdBlock is starting to have the beginnings of a test suite in /tools/test, I think the "prototype" way would be better for new code, so that every method is unit testable if we want.

Style

You typed in a couple of places:

  (function() { ... }());

but I think the spelling you want when calling an in-place anonymous function is

  (function() { ... })();

which helps the JS interpreter not get confused.

I really like your convention of prepending all jQuery variable names with a $.  I may start using that myself :).  I'd suggest you use i as a loop variable, not x, since that's pretty universal convention.

I suggest below that you remove the filter_types enumerable; but if you don't, I'd recommend renaming it to FilterTypes to conform to existing naming style for enums (e.g. in /filtering/filteroptions.js).  I've just updated https://code.google.com/p/adblockforchrome/wiki/WaysToHelpInstructions#Coding_style since it didn't mention enums before now.

JS has built-in array.forEach(), array.filter(), and array.map() in Chrome and Safari; there's usually no need to use the jQuery versions (e.g. on line 213).

I would rename "chkbox" to "chkboxType" or something, to make it clear that it doesn't represent a DOM element.

You can drop the "global_" prefix now that global_cached_subscriptions isn't global.  (In "prototype" style, this would become "this._cached_subscriptions".)

Tactics

It feels like there's a more straightforward way to do what insert_option is doing.  Maybe you could replace insert_option with something like

  var i;

  for (i=0; i < options.length; i++) {

    if (options.eq(i).data("values").index > 

        option.data("values").index) {

      break;

    }

  }

  options.eq(i).before(option);

This relies on $language_select.find("option") returning results in DOM order, but I think .find() does that, right?

I'm a little confused about what you use to sort checkboxes and <option>s, but: you might want to use the localized language name as the key to sort languages upon, rather than the fixed order returned by get_subscriptions_minus_text.  After all, in Tagalog, the "Chinese" filter list doesn't necessarily come alphabetically before the "Dutch" filter list.  You could maybe accomplish this by using the translated name as .data("values").index, and using that value to tie the <option>s to their corresponding checkboxes.

Based on how you use filter_array, I think it might be clearer to combine it with filter_types to make a map, e.g.

  var filterlist_sections = {

    'language': {

      array: language_filters,

      container: $("#language_list")

    },

    ...

  }

unless I am missing some reason that you need filter_array to be ordered.  I noticed this possible refactor because you hard coded 'filter_array[1]' at one point, which smells like you want to refer to 'filterlist_sections["language"]' instead.

That change would then make it convenient to delete the [adblock|language|other|custom]_filters variables, since you can refer to them directly now as filterlist_sections['other'].array.

Hope this helps!  Please feel free to debate any of my suggestions.

Michael