saebekassebil / teoria

Javascript taught Music Theory
http://saebekassebil.github.io/teoria
MIT License
1.32k stars 114 forks source link

Added TeoriaScale.name and TeoriaNote#atDistance #3

Closed lukehorvat closed 12 years ago

lukehorvat commented 12 years ago

TeoriaScale.name

I have encountered situations where I created a TeoriaScale object, and then later on wanted to refer to the name of the scale originally supplied to the constructor, but had no means of doing so. So I added a name field to the TeoriaScale class, which is automatically assigned a value in its constructor.

So if you supply a scale name string to TeoriaScale's constructor, it will simply set the name field to that string.

Example:

teoria.scale('c', 'mixolydian').name; //returns "mixolydian"

And if you supply an array of intervals as the scale to TeoriaScale's constructor, it will lookup the name based on whether a matching array can be found in teoria.scale.scales. The lookup is O(N^2) because you are storing the scale arrays in an object literal. Whether that is acceptable or not is up to you, but since there aren't that many scales, I think it is.

Example:

teoria.scale('c', ['M2', 'M3', 'P5', 'M6']).name //returns "majorpentatonic"

And obviously if you specify a scale array that doesn't match any of the arrays in teoria.scale.scales, the name field remains undefined, which is expected behaviour in my opinion. The alternative would be to make the constructor raise an error when it cannot find a matching scale array, but I figured maybe you still want to allow people to create custom scales that are not included in teoria.scale.scales, so I decided not to do that. If that isn't the case, then raising an error would be a nice way of preventing TeoriaScale objects from being constructed with no name field defined.

Example:

teoria.scale('c', ['M2', 'm3', 'P4', 'm7']).name //returns undefined

However, it should be noted that if you specify an array that matches multiple scale arrays in teoria.scale.scales (e.g. a scale that is aeolian would also be minor), only the first match will set the name field. Again, you'll need to decide whether that is acceptable.

Example:

teoria.scale('c', 'minor').name //returns "minor"
teoria.scale('c', 'aeolian').name //returns "aeolian"
teoria.scale('c', ['M2', 'm3', 'P4', 'P5', 'm6', 'm7']).name //always returns "minor"

TeoriaNote#atDistance(semitoneDistance)

It was becoming annoying repeatedly doing teoria.note.fromKey(note.key() + n) whenever I wanted to get the note n semitones from note, so I added the atDistance convenience method to TeoriaNote.

Example:

teoria.note('c4').atDistance(2) //returns d4
teoria.note('c4').atDistance(-4) //returns g#3
saebekassebil commented 12 years ago

Hey @LukeHorvat, thanks for taking interest in the project!

I'm glad that you've found this shortcoming in the library, and want to improve it - Thanks! However, I'd like you to submit separate pull requests for each of your feature implementations, as I'm trying to make the source-flow proper from the beginning.

I haven't implemented the atDistance method, because I find it "wrong" to instantiate notes, from "semitones". For example there's the problem with two notes, same in sound, but different in music-theoretical meaning:

teoria.note('c4').atDistance(-1)

Ought it to be teoria.note('b') or teoria.note('cb')?

However since the functionality already exists (teoria.note.fromKey), it might be plausible to implement this sugar method (as there's already a lot of sugar ;) )

So wrapping up: Thanks for the PR! I'd really like you to submit the atDistance method! I've implemented a simpler version of the TeoriaScale.name scale recognition in commit 03d21e7, but thanks for the idea!

lukehorvat commented 12 years ago

No problem. Thank @GregJ, he made me aware of Teoria.

I agree with your concerns for atDistance (not even sure if that is the best name for the function, but I digress). There are already a few sugar functions, probably best to keep the codebase sparse and not overdo it. When I get around to it I'll submit a pull request, but honestly it's not a big issue. teoria.note.fromKeystill works.

TeoriaScale.name was the feature I needed more. Thanks for implementing it.

gregjopa commented 12 years ago

Hey @saebekassebil, I finally got a chance to review teoria.js and its a great music theory library. It has a nice clean design and a lot of features that music.js doesn't have. I also checked out subito.js and I am a big fan. I hope to contribute to it in the near future.

saebekassebil commented 12 years ago

@GregJ: Sounds great! It'd be great with more contributors!

And let it be clearly stated that I encourage all to submit both issues and suggestions to features (as issues)!