Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm pulling data from a list of images and passing it into an information container after click. After looking at my logic I assume there is a better way to do this. Any ideas would be helpful.

var getData = function() {
  $('.guest-image').html('<img src="' + $($this).data("image") + '" />');
  if ($($this).data("known1") != undefined) {
    $('.guest-data .known-for li').eq(0).html('<img src="' + $($this).data("known1") + '" data-title="' + $($this).data("known1title") + '" />');
  }
  if ($($this).data("known2") != undefined) {
    $('.guest-data .known-for li').eq(1).html('<img src="' + $($this).data("known2") + '" data-title="' + $($this).data("known2title") + '" />');
  }
  if ($($this).data("known3") != undefined) {
    $('.guest-data .known-for li').eq(2).html('<img src="' + $($this).data("known3") + '" data-title="' + $($this).data("known3title") + '" />');
  }
  $('.guest-data .title').text($($this).data("name"));
  $('.guest-data .desc').text($($this).data("desc"));
  $('.guest-data .link').text($($this).data("link"));
  $('.guest-data .link').attr('href', $($this).data("link"));
}
share|improve this question
add comment

1 Answer

squished and optimized, the code looks like this:

function getData() {
  var guestData = $('.guest-data'),
    knownFors = $('.known-for li', guestData),
    data = $($this).data();
  $('.guest-image').html('<img src="' + data.image + '"/>');
  $.each(data.knowns, function (i, entry) {
    knownFors.eq(i).html('<img src="' + entry.known + '" data-title="' + entry.title + '"/>')
  });
  $('.title', guestData).text(data.name);
  $('.desc', guestData).text(data.desc);
  $('.link', guestData).text(data.link).attr('href', data.link)
}

and here's the explanation

//named functions are better in debugging
function getData() {

  //cache frequently used elements
  var guestData = $('.guest-data'),

  //providing a query context limits jQuery to locate a selector only under the context
  //so in this code, we find .known-for li only under guestData which we already cached
  //instead of traversing the DOM again for each find
    knownFors = $('.known-for li', guestData),

    //jQuery data can be fetched as a JS object by a parameter-less .data()
    data = $($this).data();

    //by informal convention, variables prefixed by $ usually means it
    //references a jQuery object. You may not need wrap it again with
    //a $() call if you are sure it's already a jQuery object

  //there are two ways to build elements in jQuery

  //for performance, string assembly would be faster
  $('.guest-image').html('<img src="'+data.image+'"/>');

  //for readability, but slow, the jQuery element builder method
  $('.guest-image').html($('<img/>', {
    'src': data.image
  }));

  //I also have to note as to why .guest-image was not cached
  //it's simply because it is only used once in the function

  //for your "knowns", I suggest placing them in an array and in order
  //so that you can easily iterate through and attach to your html

  //example of the structure
  /*
  data.knowns = [
    {
      known : 'known here',
      title : 'title here'
    },{
      known : 'known here',
      title : 'title here'
    }
  ];
  */

  //looping through the structure
  $.each(data.knowns, function (i, entry) {

    //string assembly method
    knownFors.eq(i).html('<img src="'+entry.known+'" data-title="'+entry.title+'"/>')

    //builder method
    knownFors.eq(i).html($('<img/>', {
      'src': entry.known,
      'data-title': entry.title
    }));

  });

  $('.title', guestData).text(data.name);
  $('.desc', guestData).text(data.desc);

  //lastly, some jQuery functions return the element they operated on
  //this makes chaining possible.
  $('.link', guestData).text(data.link).attr('href', data.link);
}

Here's a JS perf performance test. Compared to your code, the builder method is more readable, verbose but 23% slower. The string assembly method is 7% faster than your code.

Seems like it was faster only on my Firefox 18 and Firefox 21 Android (Nightly) and was too eager to conclude.

share|improve this answer
    
How many time did you run your JS perf, I've been running them a few times in FF, Chrome & IE and the original code seams faster? –  anderssonola Jan 29 '13 at 15:11
    
@soderslatt well, i ran it several times and im on linux so no safari or IE to use. However, optimization isnt necessarily just abot speed. It's also about DRY and minimal code as well as readability and maintainability. Though the original code is faster, the code is all over the place. –  Joseph the Dreamer Jan 30 '13 at 12:08
1  
Of course DRY is preferred and your structure is much better than the original! Just wanted to point out that the performance figures shouldn't be taken as a fact. –  anderssonola Jan 30 '13 at 12:35
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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