Piano, output all the keys and give a scale

Question

I've two snippets below, the first one output all the keys of a piano and the second one give me a scale when given a starting note. For those who don't know how a piano is structured here is an image of a piano. Basically I want to put each notes, in an ordered array. As you can see the black notes have two names, one with b in it and one with #. Those two names are equivalent but I'm using the one with b.

So I have to create this array (like the image):

[ "A0", "Bb0", "B0", "C1", "Db1", "D1", "Eb1", "E1", "F1", "Gb1", "G1", "Ab1", "A1", "Bb1", "B1".............., "B7", "C8"]

Or check the console output from the jsfiddle below.

---

jsfiddle

const KEYS_NORMAL =  ["C", "Db", "D", "Eb", "E", "F", "Gb", "G", "Ab", "A", "Bb", "B"];

let keys = KEYS_NORMAL.slice(-3);
keys = keys.map( k => k + '0');

for(let octave = 1; octave < 8; octave++){
  for(let key of KEYS_NORMAL){
    keys.push(key + octave)
  }
}
keys.push('C8')
keys.forEach(k => console.log(k));
// I'm actually doing some work with those notes, So ultimately this is one more loop.

---

The second algorithm returns a scale given a note. For those who don't know a scale starts on any note and is given by the algorithm below:

Major: 2 - 2 - 1 - 2 - 2 - 2 - 1

Minor : 2 - 1 - 2 - 2 - 1 - 2 - 2

We can distinguish between major and minor because an m is appended to the name, eg: A is major whereas Am is minor.

Where 1 means go one note above, and 2 means go 2 notes above (the black keys are also notes).

Here is jsfiddle

Code :

 majorSemiTones = [2,2,1,2,2,2];
 minorSemiTones = [2,1,2,2,1,2];
 KEYS = ["A", "Bb", "B", "C", "Db", "D", "Eb", "E", "F", "Gb", "G", "Ab"];
 KEYS_NORMALIZED = ["C", "Db", "D", "Eb", "E", "F", "Gb", "G", "Ab", "A", "Bb", "B"];

 function getScale(scaleName){
        let tonic = scaleName.replace("m", "");
        let keys = [];
        let minor = false;
        let scaleIndex;
        let intervals = this.majorSemiTones;
        if(scaleName.indexOf("m") > -1){
            minor = true;
            intervals = this.minorSemiTones;
        }
        scaleIndex = KEYS.indexOf(tonic);// starts on the tonic
        for(let i = 0; i < 7; i++){
            keys.push(KEYS[scaleIndex]);
            if( scaleIndex + intervals[i] > KEYS.length - 1 ){
                scaleIndex = scaleIndex + intervals[i] - KEYS.length;
            }else{
                scaleIndex += intervals[i];
            }
        }
        return keys;
    }

---

What I hope to get from this review is ways to improve my code. The second snippet seems needlessly complex and unreadable while the first one seems inefficient and seems like it could benefit from some functional js.

Answer 1

Since you tagged this question with typescript, I would say you could benefit greatly from making your keyboard and scale representations into classes.

---

Since there are different keyboard layouts, I wonder if it makes sense to generalize the functionality on creating a keyboard based on standard keyboard configurations?

Your current logic to build keys seems basically hard-coded for standard 88 key keyboard. There is an odd mix of work that is happening before the main loop where keys are populated (setting the octave 0 keys) and after that main loop (setting the octave 8 key). I would strive to set all key values within the loop so this logic isn't disjointed like it currently is.

---

In both pieces of logic, you might consider "rotating" the array of 12 notes to get to an appropriate starting position for working with the array based on the values input. This could make your array interaction logic much cleaner, by avoiding the need to look for boundary conditions on the array (i.e. when you need to go back to index 0);

---

You also have some fragility in your code around data input. You really don't have anything here that validates or handles edge cases in input data (for example what if values like a, ABm, are passed to getScale()?).

Code like this:

if(scaleName.indexOf("m") > -1){

might be better to be made more specific like this:

if(scaleName.indexOf("m") === 1) {

as there is only one index value where m should ever be encountered.

Alternately, I would consider a regex like /^([A-G]b?)(m?)$/ to be able to validate the input string and capture the components of the string in one pass.

---

I don't understand the need for both KEYS and KEYS_NORMALIZED in the scale code. This is just the same array rotated, and considering you will have to rotate or transform the array given the specific input scale, why have two arrays that hold the same values?

---

Some variable names seem confusing. For example:

• In the "piano" code, your KEYS constant might better be called notes or keyNotes or similar since this array is not the representation of the physical "keys" on the keyboard.
• In getScale(), keys array is not dealing with the key of the scale or keys on a keyboard, but rather the notes in the scale, so perhaps notes might be a better name.
• In getScale() the major/minorSemiTones variables might better be named major/minroSemiToneIntervals.

---

Putting it all together might yield something like:

class Keyboard {
     static readonly standardKeyboardConfig = {
         49: {
             startKey: 'C',
             startOctave: 0,
             octaveBoundary: 'C'
         }
         61: {
             startKey: 'C',
             startOctave: 0,
             octaveBoundary: 'C'
         },
         76: {
             startKey: 'E',
             startOctave: 0,
             octaveBoundary: 'C'
         },
         88: {
             startKey: 'A',
             startOctave: 0,
             octaveBoundary: 'C'
         }
     }
     static readonly defaultKeyCount = 88;
     static readonly keyNotes = ["C", "Db", "D", "Eb", "E", "F",
         "Gb", "G", "Ab", "A", "Bb", "B"];
     readonly keys: string[];

     constructor(keyCount: number) {
         keyCount = keyCount || Keyboard.defaultKeyCount;
         if (keyCount in Keyboard.standardKeyboardConfig === false) {
             throw new RangeError('Invalid keyboard specified');
         }

         let config = Keyboard.standardKeyboardConfig[keyCount]; 
         let startKey = config.startKey;

         // first rotate the array of notes if needed
         let keyNotes = Keyboard.keyNotes;
         if(startKey !== keyNotes[0]) {
             let index = keyNotes.indexOf(startKey);
             keyNotes = keyNotes.slice(index, keyNotes.length)
                              .concat(keyNotes.slice(0, index));
         }

         // now build out keys with octave values
         let octave = config.startOctave;
         let octaveLength = keyNotes.length;
         let octaveBoundary = config.octaveBoundary;
         for (i = 0; i < keyCount; i++) {
             let noteIndex = i % octaveLength;
             let note = keyNotes[noteIndex];
             if (i > 0 && note === octaveBoundary) {
                 octave++;
             }
             this.keys.push(note + octave);
         }
     }
}

// usage for default keyboard (a piano)
let piano = new Keyboard();
let keys = piano.keys;

class Scale {
    static readonly majorSemiToneIntervals = [2,2,1,2,2,2];
    static readonly minorSemiToneIntervals = [2,1,2,2,1,2];
    static readonly regex = /^([A-G]b?)(m?)$/;
    static readonly allNotes = ["A", "Bb", "B", "C", "Db", "D", "Eb",
        "E", "F", "Gb", "G", "Ab"];
    readonly name: string;
    readonly tonic: string;
    readonly isMinor: boolean = false;
    readonly notes: string[];

    constructor (scaleName: string) {
        // validate scale name and capture components
        let match = Scale.regex.match(scaleName);
        if(match === null) {
            throw new RangeError('Invalid scale name.')
        };           
        this.name = match[0];
        this.tonic = match[1];
        let intervals = Scale.majorSemiToneIntervals;
        if(match[2].length === 1) {
            this.isMinor = true;
            intervals = Scale.minorSemiToneIntervals;
        }

        // rotate the allNotes array
        let notes = Scale.allNotes;
        let index = notes.indexOf(this.tonic);
        if (index !== 0) {
            let notes = notes.slice(index, notes.length)
                             .concat(notes.slice(0, index));
        }

        // build notes for scale based on intervals;
        this.notes.push(notes[0]);
        noteIndex = 0;
        intervalIndex = 0;
        while (intervalIndex < intervals.length) {
            this.notes.push(notes[noteIndex]);
            noteIndex = noteIndex + intervals[intervalIndex];
            intervalIndex++;
        }
    }   
}

// usage
let scale = new Scale('C');
let notesInScale = scale.notes;         

Is this more code? Yes. But, this code is more reusable, less fragile, and does not pollute the global namespace as your current code examples do. All logic is encapsulated within the classes.

Answer 2

Review

(1) Algorithm

Since the octave and the note name are completely independant, you could mitigate boiler-plate concatenation and optimize reusability by using a class that stores the information for you. Additional complexity is required to determine the accidentals of a note given its degree and pitch. I have added the algorithm for this in my alternative solution.

const KEYS_NORMAL =  ["C", "Db", "D", "Eb", "E", "F", "Gb", "G", "Ab", "A", "Bb", "B"];
let keys = KEYS_NORMAL.slice(-3);
keys = keys.map( k => k + '0');

class Note {
    constructor(degree, pitch) {
        this.degree = degree;
        this.pitch = pitch;
    }
    get Octave() {
        return this.modulo(this.pitch);
    }
    set Octave(n) {
        this.pitch = n * 12 + this.modulo(this.pitch);
    }
    modulo(n) {
        return (n % 12 + 12) % 12;
    }
    ..
}

(2) Algorithm

I like the idea of using interval patterns to identify scales.

majorSemiTones = [2,2,1,2,2,2];
minorSemiTones = [2,1,2,2,1,2];

However, when hardcoding them in the algorithm, there is not much room for extensibility and modularity.

if(scaleName.indexOf("m") > -1){
            minor = true;
            intervals = this.minorSemiTones;
        }

Alternative

Given your expected outcome:

So I have to create this array (like the image):

[ "A0", "Bb0", "B0", "C1", "Db1", "D1", "Eb1", "E1", "F1", "Gb1", "G1", "Ab1", "A1", "Bb1", "B1".............., "B7", "C8"]

The second algorithm returns a scale given a note. For those who don't know a scale starts on any note and is given by the algorithm below:

Major: 2 - 2 - 1 - 2 - 2 - 2 - 1
Minor : 2 - 1 - 2 - 2 - 1 - 2 - 2

I have created a little API to get you the scale given the input key and scale interval pattern.

Fiddle that outputs to the browser console

function getScaleByIntervalPattern(key, intervalPattern) {
  "use strict";

  const keyNote = Note.FromName(key);
  var currentNote = keyNote;

  var result = intervalPattern.reduce(function (scale, scaleStep, degree) {
    scale.push(currentNote.enharmonic().Name);
    currentNote = currentNote.clone().transpose(new Interval(1, scaleStep));
    return scale;
  }, []);

  return result;
}

This results in the following scales, using enharmonic note names. If you don't want to use the enharmonic names, you should substitute currentNote.enharmonic().Name with currentNote.Name. Or - in your case - if you want only flattened altered notes, use currentNote.descendingEnharmonic().Name.

function onLoad() {
  "use strict";

  // C# major scale
  console.log(getScaleByIntervalPattern('C#', [2, 2, 1, 2, 2, 2, 1]));
  // Db major scale
  console.log(getScaleByIntervalPattern('Db', [2, 2, 1, 2, 2, 2, 1]));
  // C# minor scale
  console.log(getScaleByIntervalPattern('C#', [2, 1, 2, 2, 1, 2, 2]));
  // Db minor scale
  console.log(getScaleByIntervalPattern('Db', [2, 1, 2, 2, 1, 2, 2]));
}

results

by scale function

- (7) ["C#5", "D#5", "E#5", "F#5", "G#5", "A#5", "B#6"]
- (7) ["Db5", "Eb5", "F5", "Gb5", "Ab5", "Bb5", "C6"]
- (7) ["C#5", "D#5", "E5", "F#5", "G#5", "A5", "B5"]
- (7) ["Db5", "Eb5", "Fb5", "Gb5", "Ab5", "Bbb5", "Cb5"]

enharmonic

- (7) ["C#5", "D#5", "F5", "Gb5", "Ab5", "Bb5", "C6"]
- (7) ["Db5", "Eb5", "F5", "Gb5", "Ab5", "Bb5", "C6"]
- (7) ["C#5", "D#5", "E5", "F#5", "G#5", "A5", "B5"]
- (7) ["Db5", "Eb5", "E5", "F#5", "G#5", "A5", "B5"]

descending enharmonic (how you'd prefer)

- (7) ["Db5", "Eb5", "F5", "Gb5", "Ab5", "Bb5", "C6"]
- (7) ["Db5", "Eb5", "F5", "Gb5", "Ab5", "Bb5", "C6"]
- (7) ["Db5", "Eb5", "E5", "Gb5", "Ab5", "A5", "B5"]
- (7) ["Db5", "Eb5", "E5", "Gb5", "Ab5", "A5", "B5"]

ascending enharmonic

- (7) ["C#5", "D#5", "F5", "F#5", "G#5", "A#5", "C6"]
- (7) ["C#5", "D#5", "F5", "F#5", "G#5", "A#5", "C6"]
- (7) ["C#5", "D#5", "E5", "F#5", "G#5", "A5", "B5"]
- (7) ["C#5", "D#5", "E5", "F#5", "G#5", "A5", "B5"]