3
\$\begingroup\$

I've been steadily improving a show many move one carousel sample that I found a few weeks ago. I want to post my improvements for others to use but don't want to spread bad code. I'm not an expert in JavaScript but I always like to produce clean code.

Working demo

(function(){
  $('.carousel-showmanymoveone .item').each(function(){
    var itemToClone = $(this);

    for (var i=1;i<4;i++) {
      itemToClone = itemToClone.next();

      // wrap around if at end of item collection
      if (!itemToClone.length) {
        itemToClone = $(this).siblings(':first');
      }

      // grab item, clone, add marker class, add to collection
      itemToClone.children(':first-child').clone()
        .addClass("cloneditem-"+(i))
        .appendTo($(this));
    }
  });
}());

Thoughts

I've done all the improvements I can think of but:

  • I'm wondering about seeing $(this) appear twice.
  • Are the siblings, children, :first-child, :first usages the best options?
  • Any other improvements or best practices I've missed?

Is there something I could do better?

\$\endgroup\$
  • \$\begingroup\$ I'm satisfied siblings is the best option \$\endgroup\$ – rtpHarry Mar 27 '15 at 10:04
  • \$\begingroup\$ Looks pretty clean for me. the only improvement that I will make , will be extract in a varible, after .clone() \$\endgroup\$ – vmariano Mar 27 '15 at 13:27
1
\$\begingroup\$

Thanks for looking at this. I have concluded that I am happy with the code.

To incorporate the valid suggest by @vmariano, I have split this off into its own variable. This does make sense from a best-practices viewpoint as once I clone the item it is no longer itemToClone it is a clonedItem.

Final code:

(function(){
  $('.carousel-showmanymoveone .item').each(function(){
    var itemToClone = $(this);

    for (var i=1;i<4;i++) {
      itemToClone = itemToClone.next();

      // wrap around if at end of item collection
      if (!itemToClone.length) {
        itemToClone = $(this).siblings(':first');
      }

      // grab item, clone, add marker class, add to collection
      var clonedItem = itemToClone.children(':first-child').clone();
      clonedItem.addClass("cloneditem-"+(i))
         .appendTo($(this));
    }
  });
}());
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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