1
\$\begingroup\$

I have this function that works fine, but it looks really ugly and repetitive...

Basically I have a featured profile and underneath I have 4 smaller thumbnails, on click I want to populate the large profile. The code works fine it just doesn't look too good...

$('ul.teamProfiles li').click(function () { 

    var $name = $(this).find('h4.name').html();
    var $picture = $(this).find('img').data('bigimg');
    var $biography = $(this).find('.biographyParas').html();
    var $jobTitle = $(this).find('p.jobTitle').html();
    var $telephone = $(this).find('p.telephone').html();
    var $linkedin = $(this).find('a.linkedinLink').attr('href');
    var $twitter = $(this).find('a.twitterLink').attr('href');
    var $mailto = $(this).find('a.mailtoLink').attr('href');

    $('.profileDetail .largeImg').attr('src', $picture);
    $('.profileDetail h3.name').html($name);
    $('.profileDetail h4.jobtitle').html($jobTitle);
    $('.profileDetail .biographyContainer').html($biography);
    $('.profileDetail span.telephone').html($telephone);
    $('.profileDetail a.linkedin').attr('href', $linkedin);
    $('.profileDetail a.twitter').attr('href', $twitter);
    $('.profileDetail a.email').attr('href', $mailto);
});
\$\endgroup\$
1
  • \$\begingroup\$ I don't like the a.linkedinLink etc. selectors. A rule in CSS is to use specificity so removing the target element type would be the first thing I do; i.e. .linkedin-link \$\endgroup\$
    – megawac
    Commented Jun 23, 2014 at 16:59

1 Answer 1

4
\$\begingroup\$

It's difficult to really clean it up, since a lot of it is "manual" translation from one piece of markup to another. Different elements, different attributes etc.

However, there's the low-hanging fruit (and golden rule): Don't repeat yourself.

So don't call $(this) over and over. It's just inefficient. Do something like

var source = $(this),
    target = $(".profileDetail");

and then chain off of those, like source.find('h4.name')...

However, a nicer way to do this might be to use a data-source/data-target attributes on the "big" profile elements. For instance

<div class="profileDetail">
  <img class="largeImg" data-source="img, data, bigimg" data-target="src">
  <h3 class="name" data-source="h4.name"></h3>
  ...

So in each element data-source is a comma-separated list of the selector to run in the source element, the (optional, defaults to html()) method to call on the result, and the (optional) arguments. Meanwhile, data-target is can be an attribute name, but it's optional and defaults to html()

Then, in the JS, do something like this:

$('ul.teamProfiles li').click(function (event) {
  var selection = $(this),
      destination = $(".profileDetail:first");

  // find and loop through elements with a data-source attribute
  destination.find("[data-source]").each(function () {
    var target = $(this),             // an element in the "big" profile container
        data = (target.data('source') || "").split(/\s*,\s*/), // parse the data-source attribute
        selector = data[0],           // get the selector
        method = data[1] || 'html',   // get the method (defaults to html)
        args = data.slice(2),         // slice off the arguments
        attr = target.data('target'), // get the destination attribute (if any)
        source, content;

    if(!selector) return; // nothing to do
    source = selection.find(selector); // find the source element in the small profile
    content = source[method].apply(source.get(0), args); // call the method with the args
    if(attr) {
      target.attr(attr, content); // set the target's attribute, if that's what's needed
    } else {
      target.html(content); // otherwise, default to inserting the content as html
    }
  });
});

That function is pretty generic (though it won't work for more complex stuff like setting a bunch of css).

The point is, that the mapping of small profile to large profile can be declared entirely in the HTML; the JS doesn't need to change if you add/remove elements.

Haven't tested it, but it should work

\$\endgroup\$
4
  • 2
    \$\begingroup\$ One little convention nitpick, people like to prefix $ to cached jQuery objects. E.g. var $this = $(this) \$\endgroup\$
    – megawac
    Commented Jun 23, 2014 at 16:56
  • 1
    \$\begingroup\$ @megawac Sure - personally, though, I dislike it. $this isn't any more descriptive than this; I prefer to just use a more descriptive name. Whether it's a jQuery-wrapped element or not, it's just a variable. The $-prefix is a sort of Hungarian notation that I just personally don't like, unless you need to disambiguate a "raw" element and a jQuery-wrapped one. Besides, all those dollar-signs makes me think of PHP, and then I start to feel sad... Feel free to use it yourself, though :) \$\endgroup\$
    – Flambino
    Commented Jun 23, 2014 at 17:03
  • \$\begingroup\$ Just a convention, I hated it at first. Now I typically prefix any element wrapper (e.g. a dojo element) with $ as most devs will instantly understand what the variable is \$\endgroup\$
    – megawac
    Commented Jun 23, 2014 at 17:06
  • 1
    \$\begingroup\$ @megawac Oh, I get why it's a convention, and the reasons are sound enough. I just can't get myself to like it. Besides my PHP traumas, I just think that it's weird to put jQuery (or Dojo, etc.) on a pedestal where it needs its own special naming convention. Otherwise, go all-in on Hungarian notation. But then again, that doesn't make sense in a dynamically-typed language. I just rarely find it useful (or pretty) to add all those extra $; it feels more like visual noise than anything helpful. If I see one $(..) call, I can pretty much assume it's jQuery anyway. \$\endgroup\$
    – Flambino
    Commented Jun 24, 2014 at 0:16

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.