Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The script is for rotating 5 different coloured gears (tandwiel) on my site.

I think the code can be improved and improved performance. 5x var paper isn't that smart for speed?

Getting the current code working faster in the normal state is the primary concern, but there are two smaller issues I know of that I would appreciate help with:

  1. The code works most of the time, but when changing the browser size I have a problem with the rotating left and right: 1 gear will rotate the other direction, but starts to rotate very fast and finally slows down to normal speed. The longer you let it rotate at normal speed and then change browser, the longer the gear rotates too fast. It seems to me it rotates backwards all the missing angles til it will be normal again??
  2. How to stop the JavaScript when the browser size is mobile. So it won't overload for mobile. The animation should start again when changing to bigger browser size.

Those two issues are not the primary concern.

var w = window.innerWidth;

if (w>=960) {window.direction = -1; }
else {window.direction = 1; }

window.onresize=function(){
var w = window.innerWidth;
if (w>=960) {window.direction = -1; }
else if (w<960 && w>=715){window.direction = 1; }
};

window.onload = function() {  
var paper = new Raphael(document.getElementById('canvas_container'), 730, 750); 

var paper = new Raphael(document.getElementById('tandwiel-1'), 240, 240);
var tandwiel1 = paper.image( "tandwiel-1.png",0,0,240,240); 

var paper = new Raphael(document.getElementById('tandwiel-2'), 240, 240);
var tandwiel2 = paper.image( "tandwiel-2.png",0,0,240,240);

var paper = new Raphael(document.getElementById('tandwiel-3'), 240, 240);
var tandwiel3 = paper.image( "tandwiel-3.png",0,0,240,240);

var paper = new Raphael(document.getElementById('tandwiel-4'), 240, 240);
var tandwiel4 = paper.image( "tandwiel-4.png",0,0,240,240);

var paper = new Raphael(document.getElementById('tandwiel-5'), 240, 240);
var tandwiel5 = paper.image( "tandwiel-5.png",0,0,240,240);

var angle = 0;

setInterval( function() {
angle += 2;        
tandwiel1.stop().animate( { "transform": "R" + angle }, 1000, "<>" );
}, 150 );

setInterval( function() { 
angle += 2;
tandwiel2.stop().animate( { "transform": "R" + -angle }, 1000, "<>" );
}, 150 );

setInterval( function() {
angle += 2;        
tandwiel3.stop().animate( { "transform": "R" + angle }, 1000, "<>" );
}, 150 );

setInterval( function() {
angle += 2;        
tandwiel4.stop().animate( { "transform": "R" + -angle }, 1000, "<>" );
}, 150 );

setInterval( function() {
angle += 2;        
tandwiel5.stop().animate( { "transform": "R" + direction*angle }, 1000, "<>" );
}, 150 );
}
share|improve this question
    
I only just realized what you were saying in the first list point about the animation speeding up, then re-acclimating. This is caused by the animate subroutine, and if you check my first comment on my answer- I have already addressed the solution. There is a flaw in Raphael that causes this, not your script. You can only "fix" it by replacing it. If you are interested, I found a proposed solution once before, I can try and find it for you again. –  blaze4218 Jun 5 at 1:24
    
@Dick - why don't you respond when people ask you questions? I asked you both here and in your StackOverflow post why you're running a 1000ms animation and then stopping it after 150ms and you never responded to either? We can't help you on some things if this isn't a 2-way communication. –  jfriend00 Jun 5 at 4:37
    
@blaze4218: If you can find it, that would be nice. –  Dick Jun 5 at 9:35
    
@jfriend00: I am sorry about that. It was late at night in my country and was busy commenting on this post. I don't really know why I did that 1000ms animation, I found some code on the net and tewaked it to be like it is now. I am a newbie to JS and Raphael, but really want to learn it, because I definately see the nice possibilities of it. –  Dick Jun 5 at 9:38

2 Answers 2

up vote 5 down vote accepted

Ok, now that I have a firm grasp on what you're doing, I think I have what you're looking for. The below still applies in terms of optimizing what you posted, but I think what you want is more along the lines of the following:

fiddle

var paper = Raphael("canvas", 500, 500);

var rectangles = {
   1 : paper.rect(100,100,90,40).attr({fill: "#44ddff"}) ,
   2 : paper.rect(200,100,90,40).attr({fill: "#4488ff"}) ,
   3 : paper.rect(100,200,90,40).attr({fill: "#44ddff"}) ,
   4 : paper.rect(200,200,90,40).attr({fill: "#4488ff"})
};

function repeat() {

    var runOnce = 0;

    for (var i in rectangles) {
        rectangles[i].animate(
            {transform:'...R' + (i%2===0?'':'-') + '360'} ,
            2000 ,
            function() {

                runOnce++;

                if (runOnce==4)
                    repeat();

            });
    }
};
repeat();

It still needs some tweaks for your cogs, but I think this is a more adept to the capabilities of the Raphael framework.


Kudos on tackling Raphael at an early stage of learning. It's quite advanced, but definitely fun.

The first recommendation I will make is that you shouldn't repeatedly call "setInterval". It is a large waste of resources. Also, you are not creating multiple papers, you are overwriting the existing variable. Honestly, between that and the reuse of angle I'm somewhat confused this code is working for you at all... I certainly cant get it to run on fiddle.

var w = window.innerWidth;

if (w>=960) {window.direction = -1; }
else {window.direction = 1; }

window.onresize=function(){
var w = window.innerWidth;
if (w>=960) {window.direction = -1; }
else if (w<960 && w>=715){window.direction = 1; }
};

window.onload = function() {  

    var paper = new Raphael(document.getElementById('canvas_container'), 730, 750); 

    var tandwiel1 = paper.image( "tandwiel-1.png",0,0,480,240); 

    var tandwiel2 = paper.image( "tandwiel-2.png",0,0,720,240);

    var tandwiel3 = paper.image( "tandwiel-3.png",0,0,960,240);

    var tandwiel4 = paper.image( "tandwiel-4.png",0,0,1200,240);

    var tandwiel5 = paper.image( "tandwiel-5.png",0,0,1440,240);

    var angle = 0;

    setInterval( function() {

        angle += 2;        

        tandwiel1.stop().animate( { "transform": "R" + angle }, 1000, "<>" );

        tandwiel2.stop().animate( { "transform": "R" + -angle }, 1000, "<>" );

        tandwiel3.stop().animate( { "transform": "R" + angle }, 1000, "<>" );

        tandwiel4.stop().animate( { "transform": "R" + -angle }, 1000, "<>" );

        tandwiel5.stop().animate( { "transform": "R" + direction*angle }, 1000, "<>" );

    }, 150 );
}

Next I would swap out the unreliable setInterval for a recursive setTimeout. But that is not necessary for this experiment. I use setInterval plenty when whipping up little tests such as this, it's a bit easier to set up.

Additionally, have you tried using a Raphael set to perform your animation with only one animate directive? It might speed it up a bit.

UPDATE:

Upon reconsideration, I realize why it would work. Even though you are overwriting the paper variable, the object has already been added to the DOM. This is not undone when the variable is modified/removed within JavaScript. Ether way, you would want to encapsulate all of your elements on one paper if they are related/displayed-near-each-other

UPDATE:

A further consideration. What I like to do is first create the Raphael objects so that I can access them by themselves

var myObjects = {

    tandwiel1 : paper.image( "tandwiel-1.png",0,0,480,240, 

    tandwiel2 : paper.image( "tandwiel-2.png",0,0,720,240),

    tandwiel3 : paper.image( "tandwiel-3.png",0,0,960,240),

    tandwiel4 : paper.image( "tandwiel-4.png",0,0,1200,240),

    tandwiel5 : paper.image( "tandwiel-5.png",0,0,1440,240)
};

And then, if I need to animate them as a group, I create a set for easy manipulation.

var allTanwiels = paper.set(myObjects.tandwiel1 , myObjects.tandwiel2 , myObjects.tandwiel3 , myObjects.tandwiel4 , myObjects.tandwiel5)

allTanwiels.forEach(function(tanwiel) {
    tanwiel.stop().animate( { "transform": "R" + angle }, 1000, "<>" );
});
share|improve this answer
    
Thanks a lot both for the quick replies and explanations. I really appreciate that! –  Dick Jun 4 at 22:18
    
See the working part of what I have made: testsite-vc.verwey-creatives.nl/diensten/… Here you can also see the rotation reverse speed problem when rescaling the browser down. –  Dick Jun 4 at 22:19
    
That looks good. Suggest using Raphaels animateWith to make sure the frames sync up. Although, it's not without error, in fact some people recommend forgoing the built in animation and writing your own to take advantage of the browsers animationFrame for better syncing and redraw. –  blaze4218 Jun 4 at 22:25
    
In my live test site its still the code I got already. I assume that in your code you wrote the rotation of the gears is all the same? In my site the have to rotate like gears interacting eachother. –  Dick Jun 4 at 22:35
    
Yes, based on your reuse of the angle variable I mistook your intention as wanting them to rotate together. Actually, I don't see how you implement direction, so I don't think my changes would affect it... –  blaze4218 Jun 4 at 22:45

A good mindset to be in when tackling coding tasks like this is to Don't Repeat Yourself (known as DRY). If you're ever copying/pasting the same or mostly the same code in more than once place, then there's probably a better way to do what you're doing. You can either put something into a loop to repeat the same operation or put the common code in a function (with one or more arguments) and just call the common operation each time you need it. You pretty much should never copy the same code in multiple places.

Here's a shorter version with these changes:

  1. Just call window.onresize() initially rather than duplicate its code
  2. Generate the 5 tandwiel items in a loop since they are all the same code except a name which you can generate with code in the loop
  3. Store the 5 items in an array rather than named variables so they are easier to iterate over.
  4. Use only one setInterval() timer and process all five items in the array from that one loop.
  5. Switched to just using one paper object instead of 6.

And the code:

window.onresize = function () {
    var w = window.innerWidth;
    if (w >= 960) {
        window.direction = -1;
    } else if (w < 960 && w >= 715) {
        window.direction = 1;
    }
};

window.onresize();

window.onload = function () {
    var paper, images = [];
    paper = new Raphael(document.getElementById("canvas_container"), 730, 750);

    for (var i = 1; i <= 5; i++) {
        images.push(paper.image("tandwiel-" + i + ".png", 0, 0, 240, 240));
    }
    var angle = 0;

    setInterval(function() {
        angle += 2;
        for (var i = 0; i < images.length; i++) {
            images[i].stop().animate({"transform": "R" + angle}), 1000, "<>");
        }
    }, 150);
}

Additional issues/questions:

  1. Why are you starting a 1000ms animation and then repeatedly stopping it after 150ms. It seems there is a more efficient way to do that.
share|improve this answer
    
Points 2 and 3 are both no. Raphael creates an SVG object and appends it to the DOM. Done the way the OP coded it, they wouldn't actually be related. See my solution regarding reuse of the same paper object –  blaze4218 Jun 4 at 21:55
    
Thanks for your post btw! –  Dick Jun 5 at 9:45

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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