patternjavascriptMinor
Improving fading functions
Viewed 0 times
fadingfunctionsimproving
Problem
Regarding these two functions, which do the same thing...
-
Large and confusing:
-
Slighty smaller but maybe more confusing:
How could I make that simpler/cleaner/easier to read? Maybe split into more functions?
It's interesting how the 2nd function's
Maybe (the obvious) use two functions (
Update
I guess this is better:
```
fadeIn: function(){
if (volumeIntervalId) clearInterval(volumeIntervalId);
volumeIntervalId = setInterval(function(){
var vol = songInstance.getVolume();
if (vol 0){
songInstance.setVolume( vol - volumeIncrement );
}else{
songInstance.stop();
clearInterval(volum
-
Large and confusing:
var fade = function(flag){
if (volumeIntervalId)
clearInterval(volumeIntervalId);
switch (flag){
case 'in':
volumeIntervalId = setInterval(function(){
var vol = songInstance.getVolume();
if (vol 0){
songInstance.setVolume( vol - volumeIncrement );
}else{
clearInterval(volumeIntervalId);
songInstance.stop();
}
}, volumeIntervalDelay);
break;
default:
clearInterval(volumeIntervalId);
break;
}
},-
Slighty smaller but maybe more confusing:
var fade = function(flag){
if (volumeIntervalId)
clearInterval(volumeIntervalId);
volumeIntervalId = setInterval(function(){
var vol = songInstance.getVolume();
if (flag === 'in'){
if (vol 0) {
songInstance.setVolume( vol - volumeIncrement );
return;
}else{
songInstance.stop();
}
}
clearInterval(volumeIntervalId);
}, volumeIntervalDelay);
},How could I make that simpler/cleaner/easier to read? Maybe split into more functions?
It's interesting how the 2nd function's
setInterval anonymous function keeps the value of 'flag' due to the closure. Is there any risk? Should I avoid that?Maybe (the obvious) use two functions (
fadeIn and fadeOut)? Why do people (like me) tend to complicate things like that?Update
I guess this is better:
```
fadeIn: function(){
if (volumeIntervalId) clearInterval(volumeIntervalId);
volumeIntervalId = setInterval(function(){
var vol = songInstance.getVolume();
if (vol 0){
songInstance.setVolume( vol - volumeIncrement );
}else{
songInstance.stop();
clearInterval(volum
Solution
The updated code looks much better but you could try to extract some more common structure.
You can define fading in a general way that allows you to plug in some fading strategies. You already have fade in and fade out but you'll be able to reuse the same code for other 'fading'.
You can then define
I think there is still room for improvement. Your code relies on some global state through
Clearing the timed action could be a bit more tricky. One possible solution could be to make your function return another function that closes over
Disclaimer
I'm not a Javascript expert so I cannot guarantee that the code I wrote works flawlessly but I hope you at least got some ideas on how you could improve your code.
You can define fading in a general way that allows you to plug in some fading strategies. You already have fade in and fade out but you'll be able to reuse the same code for other 'fading'.
fade: function(fadingStrategy){
if (volumeIntervalId) clearInterval(volumeIntervalId);
volumeIntervalId = setInterval(fadingStrategy, volumeIntervalDelay);
}You can then define
fadeIn and fadeOut by simply passing the appropriate strategies to fade.I think there is still room for improvement. Your code relies on some global state through
volumeIntervalId and volumeIntervalDelay. Getting rid of the second one is trivial. You can just pass it as a parameter to fade, making it even more flexible as it will now support also different combinations of fading strategies and fading intervals.Clearing the timed action could be a bit more tricky. One possible solution could be to make your function return another function that closes over
volumeIntervalId. In that way you will only need to keep track of the function you got from the invocation of fade and invoke it when you want to stop its action.fade: function(fadingStrategy, volumeIntervalDelay){
var volumeIntervalId = setInterval(fadingStrategy, volumeIntervalDelay);
return function(){clearInterval(volumeIntervalId);};
}Disclaimer
I'm not a Javascript expert so I cannot guarantee that the code I wrote works flawlessly but I hope you at least got some ideas on how you could improve your code.
Code Snippets
fade: function(fadingStrategy){
if (volumeIntervalId) clearInterval(volumeIntervalId);
volumeIntervalId = setInterval(fadingStrategy, volumeIntervalDelay);
}fade: function(fadingStrategy, volumeIntervalDelay){
var volumeIntervalId = setInterval(fadingStrategy, volumeIntervalDelay);
return function(){clearInterval(volumeIntervalId);};
}Context
StackExchange Code Review Q#59423, answer score: 2
Revisions (0)
No revisions yet.