4
\$\begingroup\$

Background: I was making a chrome extension that injects movie ratings into theatres' showtimes pages. The problem I was duplicating a lot of code calling similar but just slightly different functions. Sometimes I wanted to do an $(el).prepend, sometimes $(el).append, or $(el).after, or $(el).find('something').prepend, or $(el).closest('something').prepend/append/after.

An example of an instance of a function I'm trying to generalize/abstract:

$('.FFEC-Display').each(function(i, el){
    var title = $(el).find('h3').text().replace(blacklistRegexp, '').trim();
    /* can ignore the three lines below, they are always the same
    var tomato = findTomato(title, tomatoes);
    var classes = ["poster-width", "overlay"];
    var templateData = tomato ? prepareData(tomato, classes) : { query: title, classes: namespaceClasses(classes) };
    */
    $(el).find('.movie-link').prepend(Handlebars.templates.ratings(templateData)); //this is the line that gives me trouble
});

Another instance:

$('.MovieItemsRotator .item').each(function(i, el){
    var title = $(el).find('.Title').text().replace(blacklistRegexp, '').trim();
    /* you can ignore the code in here
    var tomato = findTomato(title, tomatoes);
    var classes = ["overlay", "overlay-poster"];
    var templateData = tomato ? prepareData(tomato, classes) : { query: title, classes: namespaceClasses(classes) };
    */
    $(el).prepend(Handlebars.templates.ratings(templateData));
});

I have three more instances of very similar functions, and plan on adding more. Full code page here on GitHub

One of the half-joking answers in my SO question inspired me to write a solution using strings of function names, iteratively called on $(el) to solve the problem.

It started out as an exercise just because it seemed like too fun a solution not to try and write, but now I'm feeling more tempted to actually use the code, as opposed to this other one, which seems much more readable/maintainable. Hopefully people with more experience can give me some advice.

The code in question:

json = [
  {target: '.FFEC-Display', title : 'h3', actions : [{action: 'find', target: '.movie-link'}, 'prepend']},
  {target: '.MovieItemsRotator .item', title: '.Title', actions: ['prepend']}
];


$.each(json, function(index, j){
  $(j.target).each(function(i, el){
    var title = $(el).find(j.title).text().replace(blacklistRegexp, '').trim();
    /* you can ignore the code in here
    var tomato = findTomato(title, tomatoes);
    var classes = ["poster-width", "overlay"];
    var templateData = tomato ? prepareData(tomato, classes) : { query: title, classes: namespaceClasses(classes) };
    */
    var abomination = $(el);
    $.each(j.actions, function(k, action){
      if(typeof action === "string") {
        abomination = abomination[action]();
      } else {
        abomination = abomination[action.action](action.target);
      }
    });
    abomination(Handlebars.templates.ratings(templateData));
  });
});
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Firstly, I wouldn't say it's evil. From JS's point of view, obj["foo"]() is identical to obj.foo(). The dot-syntax and bracket-syntax are completely interchangeable. Of course, if you call your variable "abomination" then, yeah, it's going to sound evil.

Anyway, I think there's a bug or two in your code. When you loop through the json array and get to the actions property, you call the function on $(el) right away. This won't actually do anything, as far as I can tell.

Here's your code, manually interpreted, so to speak:

// this line does nothing; abomination is still === $(el)
abomination = abomination[action](); // same as: abomination = $(el)["prepend"]();

// ...

// and this line fails with "error: object is not a function", because $(el) is indeed not a function
abomination(Handlebars.templates.ratings(templateData)); // same as: $(el)(Handlebars.templates.ratings(templateData))

Also, I imagine the objects in the json array are meant to work on different sites. In that case, I'd key them to the site's domain, so you don't have to loop through everyone of the them on every site. This also eliminates the risk of "false positives" such as if a two different sites both use, say, "movie" as a class name.

Here is what I'd probably do. I've changed names here and there to make 'em more descriptive, but the comments should make it fairly clear

var sites = { // slightly more descriptive name than "json"
  'movietheatre.com': {
    container: '.FFEC-Display', // changed the name here too..
    title:     'h3',
    select:    [
      { name: 'find', args: ['.movie-link'] } // note that args is an array
    ],
    action:    'prepend'
  },

  'showtimes.randomcinema.com': { // note there's no `select` property here
    container: '.MovieItemsRotator .item',
    title:     '.Title',
    action:    'prepend'
  }
};

var domain = window.location.hostname.replace(/^www\./, ""), // the domain (incl. subdomain unless it's "www")
    config = sites[domain]; // get the config

// only do stuff if there's a configuration for the current domain
if( config ) {
  $(config.container).each(function () {
    var container = $(this);

    // I'd put the blacklist-replace/trim stuff into a `getRatings()` function
    // that does all the stuff you say is the always the same anyway. It can also
    // do the template rendering, while it's at it
    var title   = container.find(config.title).text();
    var ratings = getRatings(title); // get the rendered template

    // maybe throw in a check here, in case `getRatings` fails/refuses to return something
    if( !ratings ) { return; }

    // Now for the fun stuff
    var target = container;

    // if there's no sub-select-stuff, default to an empty array 
    // (and use the built-in forEach instead of jQuery)
   (config.select || []).forEach(function (call) {
      // use `apply()` to call the requested function with the requested args (if any)
      target = target[call.name].apply(target, call.args || []);
    });

    // now `target` should be the right element
    // and we can call the required insert/append/prepend action
    target[config.action](ratings); // i.e. target.prepend(ratings)
  });
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.