HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Improving logic for pulling and passing data();

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
passinglogicforandpullingdataimproving

Problem

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('');
  if ($($this).data("known1") != undefined) {
    $('.guest-data .known-for li').eq(0).html('');
  }
  if ($($this).data("known2") != undefined) {
    $('.guest-data .known-for li').eq(1).html('');
  }
  if ($($this).data("known3") != undefined) {
    $('.guest-data .known-for li').eq(2).html('');
  }
  $('.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"));
}

Solution

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('');
  $.each(data.knowns, function (i, entry) {
    knownFors.eq(i).html('')
  });
  $('.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('');

  //for readability, but slow, the jQuery element builder method
  $('.guest-image').html($('', {
    '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('')

    //builder method
    knownFors.eq(i).html($('', {
      '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.

Code Snippets

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)
}
//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);
}

Context

StackExchange Code Review Q#21006, answer score: 2

Revisions (0)

No revisions yet.