Gfrktyl

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.

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.

I hope you can check out my site. I wrote different instrument review and other musical information like understanding musical notes.
Here is a sample of what I wrote, this is about the piano keyboard layout.https://musicadvisor.com/piano-keyboard-layout/

Either way, thanks so much for this blog of yours, keep it up!
Oh don't forget to visit and like my page on Facebook https://web.facebook.com/musicadvisorStephanie/

Cheers,
Stephanie