Should I write my fadeTo function differently?

fadeTo = function(obj, time, to, cChange ){
if ( typeof( time ) == "undefined" )
time = 1000;
if ( typeof( to ) == "undefined" )
to = 1;

if ( time <= 0 )
return true;

objSt = obj.style;
objOpac = parseFloat(objSt.opacity) + 0;

if ( typeof( cChange ) == "undefined" )
{
cChange = (to - objOpac)/(time/10);
}

objSt.opacity = objOpac + cChange;

setTimeout( function(){
fadeTo( obj, time - 10, to, cChange);
}, 10);
};


This code was written as a replacement for fadeIn and fadeOut. I made this for learning purposes and it works fine.

I'm just not sure if I could write it better, and if it could cause problems in the future. I feel that I should use two functions instead: one as an init function, and one to finish the process without asking all the extra ifs and simple variable settings.

• typeof is an operator and not a function, also prefer === to == for consistency. Why the + 0 on objOpac ? – Benjamin Gruenbaum Nov 7 '13 at 0:36
• For dos, don'ts (especially don'ts) and tips read JavaScript: The Good Parts by Douglas Crockford. Can't recommend it enough for anyone leaning JS, and it's only ~150 pages; not some gigantic "bible"-type programming book. Also check out his site, and run your code through jslint (incidentally built by Crockford) or jshint – Flambino Nov 7 '13 at 0:40
• @BenjaminGruenbaum Much thanks for the quick response! I'll remember the === comparison operator. I used The + 0 because I read some where that parseFloat(objSt.opacity) could return a null or something like that, so add a real to make it a value. I believe it originally looked more like: (parseFloat(objSt.opacity) || 0) – Andrew Nov 7 '13 at 0:44
• @Flambino Thanks for the recommendation! I think this will be a great place for me to start! – Andrew Nov 7 '13 at 0:46
• @Lemony There are things that aren't "really numbers" like NaN if you add 0 to it you still get NaN so that doesn't help. – Benjamin Gruenbaum Nov 7 '13 at 1:00

Well written animation functions typically use a tweening algorithm that checks the system clock to see how much time is actually remaining in the originally specified time allotment and then adjusts the animation increment to put things back on time, even if the timer events weren't coming in exactly on time or the browser was super busy and you weren't getting all the cycles you might want or the host CPU just isn't very fast. Done well, this can even allow you to adjust the step value for smoother animations on more capable computers and coarser steps on less capable computers.

As you have it, you have a hard coded step function in time of 1/10th the time and thus 1/10th the total opacity change. On capable computers, you can run much, much smoother than this.

Here's an example:

function now() {
return new Date().getTime();
}

// sanitize optional arguments
if ( typeof time === "undefined" )
time = 1000;
if ( typeof to === "undefined" )
to = 1;

if ( time <= 0 )
return true;

var objSt = obj.style,
originalOpacity = parseFloat(getComputedStyle(obj, null).getPropertyValue("opacity")),
deltaOpacity = to - originalOpacity,
sanitizeOpacity,
minT = 30,
startTime = now();

// based on whether we're going up or down, set our santize function
if (deltaOpacity > 0) {
sanitizeOpacity = function(x) {
// not more than 1
return Math.min(x, 1);
}
} else {
sanitizeOpacity = function(x) {
// not less than zero
return Math.max(x, 0);
}
}

function step() {
// calculate how much time has elapsed since start
// and then calculate what the opacity should be set
// to for the animation to be perfectly on time
var elapsed = now() - startTime, completion;

// if all our time has elapsed, then just set to final opacity
if (elapsed >= time) {
objSt.opacity = to;
// we're done, call the completion function
// with the this pointer set to the object that was animating
if (doneFn) {
doneFn.apply(obj);
}
} else {
// still some time remaining
// calculate ideal opacity value to be "on time"
completion = elapsed/time;
objSt.opacity = sanitizeOpacity((deltaOpacity * completion) + originalOpacity);
// schedule next step
setTimeout(step, minT);
}

}
// start the animation
setTimeout(step, minT);
}


Working demo: http://jsfiddle.net/jfriend00/hG3Wj/

1. You MUST use var in front of variables that should be local variables.
2. You need to be getting the original opacity as the computedStyle, not just reading opacity directly because you need to include style sheet opacity, if nothing is set directly on the object.
3. If you want to know if an argument is truly undefined, you need to use === to test for it so there can be no automatic type conversions.
4. I find it more efficient to define an inner function that is called repeatedly because then all of our argument sanitization and what/not only has to happen once, not on each iteration and we can have state variables in the outer closure.
• Thanks, that was actually very insightful to my question! I will revise my function to follow a similarity and hopefully it'll run smoothly. Also, I too noticed my hard-coding mistake. It most likely happened because I was in a hurry to write this - I've been struggling with javascript for about a day and now I'm grasping it a bit better. I'll do some research on the var-- but I understand what you're saying about it. Thanks! – Andrew Nov 7 '13 at 1:15
• @Lemony - code example added to my answer. – jfriend00 Nov 7 '13 at 1:18
• Thanks! It looks great. On a side note, I noticed that if you were to quickly press the "GO" button it would flicker until changing to the last wanted opacity. So that means I would have to erase any Timers related to the object fading, right? How show I go about doing this. Saving a variable in the document div called "fadeTimer" and erasing it then set it to the new timer each time needed? – Andrew Nov 7 '13 at 1:25
• @Lemony - you have to decide what you want the behavior to be if another animation is started while one is currently running. Do you want it to stop the previous one and start the new one, ignore the new one because one is already running, start the new one only when the current one has already finished, etc...? Doing any of these will involve creating a way to safely store some state that an animation of a certain type is running so you can implement one of these behaviors. That's some more code, but it could be leveraged by multiple animation functions. – jfriend00 Nov 7 '13 at 1:29
• @Lemony - FYI, things like animation queuing is built into libraries like jQuery. – jfriend00 Nov 7 '13 at 1:29

You can more succinctly express

if ( typeof( time ) == "undefined" )
time = 1000;


as

time == null && (time = 1000);

• This is a bad suggestion. Readability of code is much more important than it being short. Tools like the closure compiler will convert simple ifs to short circuit for you anyway. – Benjamin Gruenbaum Nov 7 '13 at 1:02
• If you can't read it that says more about your own familiarity with JavaScript than the code itself. – Quolonel Questions Nov 7 '13 at 1:08
• @QuolonelQuestions Yes, but I am not very familiar with JavaScript yet. -- I do agree with Benjamin a lot; writing readable code is very important. I do think that overtime the top code would be much easier to read off again if for some reason I took a few months to learn another language and came back to that. But Still, thanks. – Andrew Nov 7 '13 at 1:20