HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Piano, output all the keys and give a scale

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theallgiveoutputkeyspianoandscale

Problem

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  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.index

Solution

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 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.')
};

Code Snippets

if(scaleName.indexOf("m") > -1){
if(scaleName.indexOf("m") === 1) {
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.to

Context

StackExchange Code Review Q#152639, answer score: 3

Revisions (0)

No revisions yet.