Skip to main content
added 96 characters in body
Source Link
George Mauer
  • 1.6k
  • 11
  • 13

One that also jumps out at me is this whole relationship between liveSounds and how you're selecting by name - without. Without more context I can't say for sure, but it seems like maybe you can have liveSounds = {} with the name being the key and that would make everything much simpler.

Also, as always, I'm going to go ahead and say the prodigious use of this here is probably entirely unnecessary and you can just use a factory method instead of a constructor.

Also, it's 2015, can't we just start using es6 already?

One that also jumps out at me is this whole relationship between liveSounds and how you're selecting by name - without more context I can't say for sure but it seems like maybe you can have liveSounds = {} with the name being the key.

Also, as always, I'm going to go ahead and say the prodigious use of this here is probably entirely unnecessary and you can just use a factory method instead of a constructor.

One that also jumps out at me is this whole relationship between liveSounds and how you're selecting by name. Without more context I can't say for sure, but it seems like maybe you can have liveSounds = {} with the name being the key and that would make everything much simpler.

Also, as always, I'm going to go ahead and say the prodigious use of this here is probably entirely unnecessary and you can just use a factory method instead of a constructor.

Also, it's 2015, can't we just start using es6 already?

Source Link
George Mauer
  • 1.6k
  • 11
  • 13

You can get really far cleaning this up by just creating some private functions and bumping them to the bottom of the file so that they can be hoisted

  this.Sound.masterVolume = function(v){ masterVolume = Math.pow(v,2);};
  this.Sound.instanceVolume = function(name, v){ instanceVolume[name] = Math.pow(v,2);};
  this.Sound.masterStop = function(){ liveSounds.forEach(reset) }
  this.Sound.masterPause = function(){ liveSounds.forEach(pause) }
  this.Sound.instanceStop = function( name ){ liveSoundsExcept(name).forEach(reset) }
  this.Sound.instancePause = function( name ){ liveSoundsExcept(name).forEach(pause) }
  this.Sound.instanceGetLive = function( name ){
    //this seems to be an error - what is match?
    return liveSounds.filter(function(snd){
      return( snd.src === match );
    });
  }
  this.Sound.masterGetLive = function(){
    return liveSounds.slice(0);//slice generates a copy - I don't think splice does
  }

  this.sound = function(name){
    var snd = new Audio(getSound(name)); //reference to `this` removed because it wasn't being used
    snd.volume = instanceVolume[name] || 1;
    snd.play = function(){ 
      //why use the ambiguous `this` here? It can vary depending on how this is called - you already have an isntance of `snd`!
      snd.volume *= masterVolume;  
      //Audio.prototype.play.apply(snd); //Why? you can just use 
      snd.play();
      liveSounds.push(snd);
      // This can't be right - you don't need to do any cleanup?
      // this.onended = function(){ liveSounds.splice( liveSounds.indexOf(snd), 1 );};
    };
    return snd;
  };

  function getSound(name) { return sounds[name].src}
  function liveSoundsExcept(name) {
    var match = getSound(name);
    return liveSounds.filter(function(snd){ return snd.src !== match });
  }
  function pause(snd) { snd.pause(); }
  function reset(snd) {
    pause(snd);
    snd.onended();
    snd.currentTime = 0;
  }

I absolutely agree with @Schism that more context is needed for a really good review but I made a few other observations inline.

One that also jumps out at me is this whole relationship between liveSounds and how you're selecting by name - without more context I can't say for sure but it seems like maybe you can have liveSounds = {} with the name being the key.

You do need to think if sounds need to get disposed of or what happens if you invoke a method for something without a name, or if there's two things with the same name.

Also, as always, I'm going to go ahead and say the prodigious use of this here is probably entirely unnecessary and you can just use a factory method instead of a constructor.