patternjavascriptMinor
Sound Control System
Viewed 0 times
soundsystemcontrol
Problem
This is my sound system that allows for the following functionality:
How could I possibly improve the code, or the system itself? I could possibly allow the user to define layers to encapsulate sectors of sound and not just the same instance. I could probably make a user.
```
// exponentially decrease sounds
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(function(snd){
snd.pause();
snd.onended();
snd.currentTime = 0;
});
}
this.Sound.masterPause = function(){
liveSounds.forEach(function(snd){
snd.pause();
});
};
this.Sound.instanceStop = function( name ){
var match = sounds[name].src;
liveSounds.forEach(function(snd){
if( snd.src !== match ) return;
snd.pause();
snd.onended();
snd.currentTime = 0;
});
};
this.Sound.instancePause = function( name ){
var match = sounds[name].src;
liveSounds.forEach(function(snd){
if( snd.src !== match ) return;
snd.pause();
});
}
this.Sound.instanceGetLive = function( name ){
return liveSounds.filter(function(snd){
return( snd.src === match );
});
}
this.Sound.masterGetLive = function(){
return liveSounds.splice();//only a copy.
}
this.sound = function(name){
var snd = new Audio( sounds[name].src ),
that = this;
snd.volume = instanceVolume[name] || 1;
snd.play = function(){
this.volume *= masterVolume;
Audio.prototy
- One master volume that controls every sound.
- A function to stop/pause every playing sound
- A function to control every instance of a sounds volume(@creation)
- A function to stop/pause every instance of the same sound
- Sounds that are playing are kept in a list to keep track of.
- retrieve a list of specified sounds playing
- Retrieve a list of every sound playing
How could I possibly improve the code, or the system itself? I could possibly allow the user to define layers to encapsulate sectors of sound and not just the same instance. I could probably make a user.
```
// exponentially decrease sounds
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(function(snd){
snd.pause();
snd.onended();
snd.currentTime = 0;
});
}
this.Sound.masterPause = function(){
liveSounds.forEach(function(snd){
snd.pause();
});
};
this.Sound.instanceStop = function( name ){
var match = sounds[name].src;
liveSounds.forEach(function(snd){
if( snd.src !== match ) return;
snd.pause();
snd.onended();
snd.currentTime = 0;
});
};
this.Sound.instancePause = function( name ){
var match = sounds[name].src;
liveSounds.forEach(function(snd){
if( snd.src !== match ) return;
snd.pause();
});
}
this.Sound.instanceGetLive = function( name ){
return liveSounds.filter(function(snd){
return( snd.src === match );
});
}
this.Sound.masterGetLive = function(){
return liveSounds.splice();//only a copy.
}
this.sound = function(name){
var snd = new Audio( sounds[name].src ),
that = this;
snd.volume = instanceVolume[name] || 1;
snd.play = function(){
this.volume *= masterVolume;
Audio.prototy
Solution
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
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
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
Also, it's 2015, can't we just start using es6 already?
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 and that would make everything much simpler.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.Also, it's 2015, can't we just start using es6 already?
Code Snippets
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;
}Context
StackExchange Code Review Q#96005, answer score: 3
Revisions (0)
No revisions yet.