exercism / rust

Exercism exercises in Rust.
https://exercism.org/tracks/rust
MIT License
1.44k stars 520 forks source link

Potential improvements to the 'scale-generator' exercise #1528

Closed isaacthefallenapple closed 9 months ago

isaacthefallenapple commented 2 years ago

I love the scale generator exercise and I've implemented something similar almost every time I learned a new programming language. It has a few issues, though, in my opinion, and I'd like to offer some suggestions on how to improve it.

The music theory

From a music theory point of view, the exercise isn't 100% accurate. It — presumably for simplicity — omits that an interval is composed of two parts: a number and a quality, not just a number of semitones. An augmented second, for example, does span three semitones just like a minor third, but it is a different interval. A second will always span two letters and a third will always span three, no matter the quality of the interval. The interval C–D# is an augmented second, the interval C–Eb is a minor third.

A related issue: scales don't have to be spelled with either flats or sharps, sometimes both are used. This is often because, if possible, a scale is most naturally written using every letter note at most once per octave. This is especially true for diatonic scales. This is why D harmonic minor is most commonly spelled D, E, F, G, A, Bb, C#, D, not D, E, F, G, A, Bb, Db, D like the exercise suggests.

Suggested improvement

Instead of notating a scale by the intervals between consecutive notes, one common way is to write it relative to the tonic's major scale. This is done by assigning each note of the major scale a degree from 1 through 7. To give a new scale you just indicate the degrees of the major scale it contains in which order, and what, if any, accidentals are applied to them. For example, to get any minor scale, you take the 3rd, 6th, and 7th degree of its relative major and flatten them by one step giving the scale 1, 2, b3, 4, 5, b6, b7. A major pentatonic scale is just its relative major, omitting the 4th and 7th degree, i.e. 1, 2, 3, 5, 6. To get a lydian scale, just raise the 4th degree: 1, 2, 3, #4, 5, 6, 7.

This generalizes really nicely to any scale and even chords can be generated like this. For example, a major chord is just 1, 3, 5, a minor chord 1, b3, 5, a dominant 7#11 chord is 1, 3, 5, b7, 9, #11 (degrees > 7 just can just be modulo'd back into the octave), etc.

Alternatively, you could stick to the consecutive interval scheme but spell the intervals out. I.e. a major scale is denoted as M2, M2, m2, M2, M2, M2, m2, a pentatonic major scale as M2, M2, m3, M2, m3, etc.

The code

A decent part of this exercise is parsing notes and intervals to and from strings. There are already a lot of dedicated parsing exercises on Exercism and I don't think it adds a whole lot to this one. I'd rather add the complexity of implementing intervals properly than parsing them.

I also think the API this exercise suggests is a little unnatural.

Suggested improvements

Shift the focus away from parsing intervals and towards implementing them in a more music theoretical way.

I think a ScaleGenerator struct that generates a given scale from a tonic on demand is more natural and Rust-y. I.e. instead of Scale::new("C", "MMmMMMm").enumerate(), something like ScaleGenerator::new("C").generate("1 2 3 4 5 6 7") would make more sense.

tl;dr

Shift the focus of this exercise away from parsing intervals and notes and more toward implementing them in all their complexity. Adjust the API to feel more natural.

I've already implemented most of these changes because, again, I really enjoy this exercise and I hadn't done it in Rust yet. I'd be happy to submit a pull request. And apply the same changes to this exercise in other languages I know.

I'd be happy to hear your feedback, especially of course @coriolinus.

coriolinus commented 2 years ago

First: thanks for the well-thought-out issue! I'm not really well-versed in the music theory aspect of things, but the things you say sound plausible to me.

The context I have is that this exercise is generated from a specification located within the problem-specifications repo. Generally, it's fine to edit track-specific implementations of an exercise, but it's even better, when there are significant changes, to upstream them so that all tracks can benefit.

It looks like your ideas here fall into two major categories: improve the music theory, and avoid string parsing. I think that the music theory portion of things properly belongs in the problem specifications, and avoiding string parsing in the Rust implementation.

I'm not going to take the lead on either of those tasks. However, I have no objection whatsoever if you want to submit a PR improving the exercise on the Rust side. Keep in mind the pedagogic goals of the exercise: https://github.com/exercism/rust/blob/22c2535ebabbb5e37b5a61e1bf1c2f4fc547f584/config.json#L1414-L1429

You can of course change those goals to some extent, but if you do, please document it there. I agree, we have a fair amount of string parsing, and we don't really need more of that to practice.

I'll look forward to the PR!

senekor commented 9 months ago

Unfortunately the scale-generator exercise was deprecated and removed.