0
\$\begingroup\$

How do I write this function more efficient? My class names always contain modell and essentially, only the numbers are changing. Is it right that I get a better performance if I update the numbers instead of adding and removing classes ?

So basically: modell[1], modell[2], ...

jQuery(document).ready(function(){
       animatemodels();
   });

   function animatemodels() {
       var classes = [ 'modell1', 'modell2', 'modell3', 'modell4', 'modell5', 'modell6', 'modell7', 'modell8', 'modell9', 'modell10', 'modell11', 'modell12', 'modell13', 'modell14', 'modell15', 'modell16', 'modell17', 'modell18', 'modell19', 'modell20', 'modell21', 'modell22', 'modell23', 'modell24', 'modell25', 'modell26', 'modell27', 'modell28', 'modell29', 'modell30', 'modell31', 'modell32', 'modell33', 'modell34', 'modell35', 'modell36', 'modell37', 'modell38', 'modell39', 'modell40', 'modell41', 'modell42', 'modell43', 'modell44', 'modell45', 'modell46', 'modell47', 'modell48', 'modell49', 'modell50', 'modell51', 'modell52', 'modell53', 'modell54', 'modell55', 'modell56', 'modell57', 'modell58', 'modell59', 'modell60', 'modell61', 'modell62', 'modell63', 'modell64', 'modell65', 'modell66', 'modell67', 'modell68', 'modell69', 'modell70'];

       jQuery(".panzoom").each(function(){
           jQuery(this).removeClass('modell1 ' + classes.join(' ')).addClass(classes[~~(Math.random()*classes.length)]);
       });

       setTimeout(animatemodels, 7000);
   }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ It's a bit unclear what your code is supposed to do. Could you perhaps include a longer descriptions of its context? \$\endgroup\$
    – jacwah
    Commented Oct 26, 2015 at 8:48

2 Answers 2

2
\$\begingroup\$

If I understand your code correctly, you are looking to remove all classes that contains modellXX (where XX is a number), then add a class with a random number within a range from {1,N}.

No need to create an array with all classes.. Use regex as shown in the following code:

function animatemodels() {
    var count = 70;
    jQuery(".panzoom").each(function(i, el) {
        // Remove all classes that start with 'modell'
        el.className = el.className.replace(/(?:^|\s)modell[0-9]*(?!\S)/g, '');
        // Add a new class with a random modell number
        el.className += " modell" + ~~(Math.random() * count);
    });
    setTimeout(animatemodels, 7000);
}
\$\endgroup\$
1
\$\begingroup\$

I'm pretty sure you have some CSS animations going on here, since your code says animate, but it doesn't include animation code. You should mention that in a comment. Also, the code is simply adding/removing classes, no animations. I think it's better you stick to making it mean about adding/removing rather than animating.

Anyways, making this shorter, jQuery's jQuery(document).ready() can be shortened to $(fn). You can also put in your entire code inside that callback instead of putting it out in the global scope. Reduces pollution.

$(function(){
  // DOM is ready
});

The next issue is what goes on when you animate. One problem is that you're creating that array each time you call animatemodels. You're also grabbing .panzoom from the DOM too. To make them more efficient, and assuming .panzoom elements don't get added/removed during the lifetime of the page, best you cache them outside animatemodels. Do the same for your interval delay of 7000, as well as the string you use to remove all your classes.

$(function(){
  var animationInterval = 7000;
  var classes = [...];
  var classNames= classes.join(' ');
  var panzooms = $('.panzoom');
  ...
});

With that out of the way, what remains in your code is the interval. You can simply inline animatemodels into setInterval.

You can also dynamically generate your classnames. Assuming they're always from 1 to n, you can use a loop. I prefer a range function to generate an array and iterate over it using map

var range = Array.apply(null, Array(5)).map(function (_, i) {return i;});
// [0,1,2,3,4]
var classes = range.map(function(i){ return 'model' + (i + 1);});
// ['model1','model2',...,'model5']

With all that, your code should look like:

$(function(){
  var animationInterval = 7000;
  var panzooms = $('.panzoom');
  var classes = range(70).map(function(i){ return 'model'+ (i + 1); });
  var classNames= classes.join(' ');

  function range(n){
    return Array.apply(null, Array(n)).map(function (_, i) {return i;});
  }

  setInterval(function(){
    panzooms.removeClass(classNames).each(function(){
      $(this).addClass(classes[~~(Math.random()*classes.length)]);
    })
  }, animationInterval);

});

Now with regards to code length, don't write very cryptic code because of code length. We have minifiers for that. Write your code verbosely for your sake and for other developers' sake. For production, run it through a minifier before deploy. Minifiers like Uglify and Closure Compiler have a feature called dead-code removal. It traces through the code, checks for code that's never reached (your mileage may vary, depending on config), and removes it.

\$\endgroup\$

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.