Improving fading functions

Question

Regarding these two functions, which do the same thing...

1. Large and confusing:

   var fade = function(flag){
   
       if (volumeIntervalId)
           clearInterval(volumeIntervalId);
   
       switch (flag){
           case 'in':
               volumeIntervalId = setInterval(function(){
                   var vol = songInstance.getVolume();
                   if (vol < 1){
                       songInstance.setVolume( vol + volumeIncrement );
                   }else{
                       clearInterval(volumeIntervalId);
                   }   
               }, volumeIntervalDelay);
               break;
           case 'out':
               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;
       }
   
   },
   

2. 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 < 1) {
                   songInstance.setVolume( vol + volumeIncrement );
                   return; 
               }
           } else if (flag === 'out'){
               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 < 1){
            songInstance.setVolume( vol + volumeIncrement );
        }else{
            clearInterval(volumeIntervalId);
        }   
    }, volumeIntervalDelay);
},

fadeOutAndStop: function(){
    if (volumeIntervalId) clearInterval(volumeIntervalId);

    volumeIntervalId = setInterval(function(){
        var vol = songInstance.getVolume();
        if (vol > 0){
            songInstance.setVolume( vol - volumeIncrement );
        }else{
            songInstance.stop();
            clearInterval(volumeIntervalId);
        }   
    }, volumeIntervalDelay);
},

Answer 1

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'.

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.