2
\$\begingroup\$

I wrote this jQuery/JavaScript code for slideshow of my dynamic webapp. Is this too much jQuery code for carousel? It seems to work fine on my PC, but with bigger images it gets slower (7MB+) which is normal I suppose? Or am I doing whole thing wrong? I know I could do it with a JavaScript for loop or something, but I think it is easier with jQuery. Is jQuery less power-efficient than plain JavaScript? I wouldnt like to change it all as it has kind of "sentimental" value for me.

$(".prev-btn").on("click", function(){
  current = $(".li-placeholder").find("li.active");
  next = current.prev()
  if (current.index() == 0 ){
    next = $(".li-placeholder li").last();
    src = $(next).attr("src");
    $(".content").attr("src", src);
    $(".download-link").attr("href", src);
    $(next).addClass("active");
    $(current).removeClass("active");
  }
  else {
  src = next.attr("src");
  $(".content").attr("src", src);
  $(".download-link").attr("href", src);
  $(next).addClass("active");
  $(current).removeClass("active");
}
})

$(".next-btn").on("click", function(){
  current = $(".li-placeholder li.active");
  next = current.next()
  if (current.index() == $(".li-placeholder li").last().index()){
    next = $(".li-placeholder li:eq(0)");
    src = $(next).attr("src");
    $(".content").attr("src", src);
    $(".download-link").attr("href", src);
    $(next).addClass("active");
    $(current).removeClass("active");


  }
  else {
  src = next.attr("src");
  $(".content").attr("src", src);
  $(".download-link").attr("href", src);
  $(next).addClass("active");
  $(current).removeClass("active");
}

})


$(".exam-span-file").on("click", function(){
  wrapper = $(this).closest(".exam-span-wrapper");
  src = $(wrapper).find("li:eq(0)").attr("src");
  $(".content").attr("src", src);
  $(".download-link").attr("href", src);
  $(".imgmodal").show();
  exam_id = $(this).closest(".parent").attr("exam-id");
  $(".imgmodal").attr("exam-id", exam_id);
  $(wrapper).find("li:eq(0)").addClass("active");
  clone = $(wrapper).find(".exam-ul").clone();
  $(".li-placeholder").html(clone);

  if ($(".li-placeholder li").length == 1 ){
    $(".prev-btn").hide();
    $(".next-btn").hide();
  }
  else {
    $(".next-btn").show();   
    $(".prev-btn").show(); 
  }
\$\endgroup\$
  • \$\begingroup\$ You should include example HTML that this operates against. \$\endgroup\$ – Mike Brant May 26 '17 at 19:56
1
\$\begingroup\$

With something as lightweight as this, I don't think you are going to see any appreciable difference in javascript-based performance between plain Javascript and jQuery. Now, downloading 7MB images is going to be relatively slow regardless as to whether you use javascript or jQUery, and my guess is that there is really no reason you should be loading images that size ever, unless you needed user to be able to zoom in on super high-quality images such that you need lots of pixels to support this.

The #1 thing you should do here, which could help performance to some extent, is to cache your jQuery collection results to variables.

There is no reason, for example that you should be querying $(".content"), $(".download-link"), etc. over and over again like you are doing. This requires querying against the DOM (which is more expensive when querying by class than by ID because you must traverse the entire DOM) every single time.

Instead you should do something like this:

$(document).ready(function () {
    var $liPlaceholder = $('.li-placeholder');
    var $content = $('.content');
    var $downloadLink = $('.download-link');
    var $imgModal = $('.imgmodal');
    // and so on..

    // and then use these collections later on
    $('.prev-btn').on('click', function () {
        var current = $liPlaceholder.find("li.active");
        // etc.
    });
});

I think you have way too much code around traversing the collection for your prev/next buttons, along with a lot of repeated code for setting/removing active class and content/download link properties. I don't think you need to be looking for the active item at all. Consider this code:

$(document).ready(function () {
    var $slides = $('.li-placeholder li');
    var $content = $('.content');
    var $downloadLink = $('.download-link');
    // default slide index to display on page load
    var slideIdx = 0;

    $('.prev-button').on('click', function() {
        slideIdx--;
        if(slideIdx < 0) {
            slideIdx = $slides.length - 1;
        }
        showSlide(slideIdx);
    });

    $('.next-button').on('click', function() {
        slideIdx++;
        if(slideIdx === $slides.length) {
            slideIdx = 0;
        }
        showSlide(slideIdx);
    });

    function showSlide(idx) {
        $slides.removeClass('active');
        var $current = $slides.eq(idx);
        var src = $current.attr('src');
        $current.addClass('active');
        $content.attr('src', src);
        $downloadLink.attr('href', src);
    }

   // set initial slide
   showSlide(slideIdx);
}

I am not exactly sure what the exam-file-span handler is doing without being able to see the code, but just looking at the way you are traversing the DOM and performing selectors here would make me think there is improvement to be had here as well.


$(".li-placeholder").html(clone);

You really should not be using html() method (which expects string) as means to inject a cloned DOM node into DOM. You should really you append() here.

$liPlaceholder.append(clone);

You have some fundamental problems with your javascript that you should address:

  • If you are going to use semicolons to separate lines of code (as most writing javascript do, though it is optional), you need to be 100% consistent about it. You have a few spots where you are missing the semicolon to terminate the line.
  • You are not properly initializing your variables using var (or let/const if in more modern versions of javascript). You really need to get into this habit if you don't your code to work unexpectedly.
  • You are indenting your code inconsistently.
  • Every single one of your else statements in this code is avoidable. You should get into the good habit of learning how to design else conditions away when possible. The fewer parallel code paths you have in your code, the less complex (and therefore easier to test and easier to maintain) it will be.
\$\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.