patrickmestabrook/sequencr — pull request #1 by bramsey (2013-02-14 21:03:49+00:00)
Title: Code review

I'll follow the lead of @zachallaun and do a comment per commit.

### Explanations by commit

**1. Changed var declarations**

This is mostly stylistic, but javascript will pull every var declaration in a lexical block up to the top and compress it into one declaration. Doing that when you write the code saves the interpreter the work. The comma at the beginning of each subsequent declaration is there to make them align properly with the first declaration when using 2 space tabs. 4 space tabs would use the comma at the end of each line.

**2. used .sequencer:nth-child(4n) selector**

I could be mistaken, but it looked like the quarter class was used simply to change some style for every 4th button. This can be done with the css nth:child selector as follows:
```css
.sequencer:nth-child(4n) { /** style goes here **/ }
```
where 4 is the multiple to affect.

**3. Added 0.1 to time var at assignment**

The same time increment was added to each use, so I compressed them into the declaration. If the sounds would need different times, then they should be separate, so no biggie here.

**4. Used toggleClass instead of manual checking**

It looked like the 'on' and 'off' classes were being used as a boolean, and off had no impact on the style, so I simply removed 'off' and changed the boolean logic to check for the presence of 'on'.

The original segment:
```javascript
var $this = $(this);
if ($this.hasClass('off')) {
$this.addClass('on').removeClass('off');
}
else {
$this.addClass('off').removeClass('on');
}
```
was mimicking the functionality of the jQuery toggleClass method.

**5. Stored sounds in an array instead of object**

I couldn't see why you were storing the sounds in an object and giving them specific keys that you used to reference later. That seemed like a bit too much coupling for my tastes, and added some redundancy.

I moved the sound objects into an array and then looped through the array when doing the checks on whether a sound should play.
```javascript
sounds = [undefined,
new Sound("audio/hihat.wav", SOUND_LEVEL),
new Sound("audio/snare.wav", SOUND_LEVEL),
new Sound("audio/kick.wav", SOUND_LEVEL)
];

// play all the sounds!
for (var i=1, l=sounds.length; i< l; i++) {
if ( buttons[i][currentStep].hasClass('on') ) {
sounds[i].play(time);
}
}
```