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);
}
-
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– jacwah2015年10月26日 08:48:02 +00:00Commented Oct 26, 2015 at 8:48
2 Answers 2
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);
}
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.