Associating degrees with notes of scales

Question

I have a JS file that creates an object with notes from a music scale when given a key (aka tonic/ note). The code works and does what I want it to do. I need a critique on the way it's written, what could be made better, quality of code — even if it's indentation. please. I've never written code so I have no frame of reference.

I'm leaving the comments in. I know the getKey() isn't necessary for what's going on, I put it in there to serve another purpose later.

(function () {
    console.time("-------------------------------------------------------------------->> mainFunction");
"use strict";


//gets key and returns tonic object
var getKey = function (key) {
    var tonic = {};
    tonic[key] = 1;
    return tonic;
};

//takes a tonic and type and returns the scale objects in an array
var getScale = function (tonic, type) {

    //chromatic scale
    var chromatic = ['c', 'c#', 'd', 'Eb', 'e', 'f', 'f#', 'g', 'Ab', 'a', 'Bb', 'b'];


    //extract key from tonic object
    var extractTonic = Object.keys(tonic)[0];

    //get position of key in chromatic array
    var positionOfTonic = chromatic.indexOf(extractTonic);


    //if positionOfTonic is > 0, split the array into two at above position
    if (positionOfTonic !== 0) {
        var arrayOne = chromatic.splice(positionOfTonic);

        var arrayTwo = chromatic.splice(0,(positionOfTonic));

        //reset chromatic to set tonic at index 0
        chromatic = arrayOne.concat(arrayTwo);
    }

    switch(type) {
        case "major":
            var majorArray = chromatic.slice(0);

            var removeValFromIndex = [1, 3, 6, 8, 10];
            for (var i = removeValFromIndex.length - 1; i >= 0; i--) {
                majorArray.splice(removeValFromIndex[i],1);
            }
            break;
        case "minor":   
            var minorArray = chromatic.slice(0);

            var removeValFromIndex = [1, 4, 6, 9, 11];
            for (var i = removeValFromIndex.length - 1; i >= 0; i--) {
                minorArray.splice(removeValFromIndex[i],1);
            }

            break;
    }


   var getScaleObject = function (array) {
                //can't create objects with values from array....try with new constructor.
                var superTonic = {};
                var mediant = {};
                var subDominant = {};
                var dominant = {};
                var subMediant = {};
                var leadingTone = {};

                 superTonic[array[1]] = 2;
                 mediant[array[2]] = 3;
                 subDominant[array[3]] = 4;
                 dominant[array[4]] = 5;
                 subMediant[array[5]] = 6;
                 leadingTone[array[6]] = 7;

                 var majorObject = {tonic, superTonic, mediant, subDominant, dominant, subMediant, leadingTone};

                 return majorObject;
    };






    if (type === "major") { 
        console.time("-------------------------------------------------------------------->> getScaleObject (major)");
        var consoleTimeForMajor = getScaleObject(majorArray);
        console.timeEnd("-------------------------------------------------------------------->> getScaleObject (major)");
        return consoleTimeForMajor;
    }
    else if (type === "minor") {
        console.time("-------------------------------------------------------------------->> getScaleObject (minor)");
        var consoleTimeForMinor = getScaleObject(minorArray);
        console.timeEnd("-------------------------------------------------------------------->> getScaleObject (minor)");
        return consoleTimeForMinor;
    }

 };







    ////////////////INPUT////////////////
    console.time("-------------------------------------------------------------------->> getKey");
  //  console.log(getKey('c'));
    console.timeEnd("-------------------------------------------------------------------->> getKey");


    console.time("-------------------------------------------------------------------->> getScale(getKey)");
    console.log(getScale(getKey('a'),'minor'));
    console.timeEnd("-------------------------------------------------------------------->> getScale(getKey)");

//    console.time("-------------------------------------------------------------------->> getScale");
//    console.log(getScale({c:1},'major'));
//    console.timeEnd("-------------------------------------------------------------------->> getScale");



}());
console.timeEnd("-------------------------------------------------------------------->> mainFunction");

/*
todo 
    rename keys to first||tonic, second||superTonic...
    build loop and presentation
    add other types
    use obj.prototype for inheritance in getKey 


bugs
    capitalizing [chromatic] elements breaks everything...splice not working?  

restrictions
    cant compare/calculate enharmonic scales - scaleObject created for a given tonic
*/

The output looks like this:

-------------------------------------------------------------------->> getKey: 7ms
-------------------------------------------------------------------->> getScaleObject (minor): 1ms
{ tonic: { a: 1 },
  superTonic: { b: 2 },
  mediant: { c: 3 },
  subDominant: { d: 4 },
  dominant: { e: 5 },
  subMediant: { f: 6 },
  leadingTone: { g: 7 } }
-------------------------------------------------------------------->> getScale(getKey): 8ms
-------------------------------------------------------------------->> mainFunction: 23ms

I kept the console.time stuff in there, I realize it makes the code look messy, I doubt also that they're accurate measurements. I get a somewhat faster execution on Node vs. the browser console but even then the time fluctuates depending on other things windows is running. Please disregard all that.

I tried to initialize my seven note objects inside getScaleObject() as I was creating them but it didn't seem to work:

var superTonic[array[1]] = 2; //didn't work

I broke that process into two steps and then returned a container object.

I haven't investigated this yet but since I'm writing this... if I change the elements inside the 'chromatic' array, the whole thing melts down.

Answer 1

I've never written code so I have no frame of reference.

I wouldn't know by looking: This is very nicely done! Congrats!

Good stuff:

• "use strict"
• Using an IIFE to contain everything
• Well-structured
• Copious comments

Typically, beginners stop once something works - usually after changing stuff again and again until errors stop appearing - and neglect "cleaning up". But you've maintained a good structure and kept things neat.

Again, good job!

All that said, when looking closer things become a little muddled, due in part to the slight detours you take in building the return object.

But before I go on with how I might refactor things, a few things I noticed while reading:

• Indentation is inconsistent in places. For instance, the body of getScaleObject is indented much more than the rest. This might be a side-effect of copy pasting things into the question, if your code has mixed spaces and tabs for indentation. Check your editor to make sure you're using one or the other; not both.

• You seem to be timing a lot of things. Really, don't worry about timing unless you really have to. I don't know your use case for this code but I doubt it's too slow. If it's fast enough, then it's fast enough - don't worry about it. (Conversely, if timing is super critical, JavaScript probably isn't the right tool to begin with.)

• You have some unnecessary parentheses, like:

  chromatic.splice(0,(positionOfTonic));
  

  positionOfTonic need not be wrapped.

  Also in that line, and a few other places, you skimp on spacing. It's good practice to separate arguments, so the line would ideally look like this:

  chromatic.splice(0, positionOfTonic);
  

  Small stuff, but greatly improves readability.

• Your switch is lacking a default case. It's allowed, but it made me stop for a moment. Of course, it's not entirely clear what should happen in case the type isn't major or minor as that doesn't really make sense. You could consider throwing an exception to alert the user that their input is invalid. However, right now the code handles it quite gracefully by simply returning undefined (by gracefully I mean that it doesn't just crash). Still, you examine the type in two places, using a switch in one place, and an if.. else if in the other. This seems like needless variation, and perhaps also needless duplication of code.
  Anyway, if you use a switch, it's good practice to add a default case, even if it only contains a comment like:

  default:
    // if we're here, type is invalid: Do nothing.
    break;
  

  Like the default case itself, the break isn't strictly necessary either, but this is like dotting the i's and crossing the t's. I'm a stickler for things like that.

• It should be noted that your code does not fail gracefully if you pass an invalid note. E.g. getScale(getKey("x"), "minor") returns a scale object, it's just not a terribly useful one.

• You also have some duplication in the code that removes notes from the (rotated) chromatic scale. Really the only difference between major and minor is which indices to remove, but you've duplicated the loop that does the removal too. Instead, you could just pick the right set of indices, and do the removal in one place.

Now, refactoring. I should note that I'm musically illiterate, so I won't speak to the correctness of the code. I'm just working backwards from the code (and referencing wikipedia), but I might miss something basic or mix up the terminology.

First, I'd make it possible to actually call your code from outside the IIFE. Right now, anytime you want to get a scale object, you have to add the code inside the IIFE's scope. This makes it impossible to use this code from other code.

The basic way would be to so something like:

var getScale = (function ()
  "use strict";
  // your code
  return getScale;
}());

That is, return the inner function, and store it on the outside, in a variable named the same.

But right now, you'd also have to somehow expose the getKey function to the outside, which complicates matters somewhat.

You could do this:

var myFunctions = (function ()
  "use strict";
  // your code
  return {
    getKey: getKey,
    getScale: getScale
  };
}());

Which would let you call myFunctions.getKey and myFunctions.getScale from the outside (note that myFunctions can be called anything; it's just an example).

But the simpler solution would probably be to just change the way getScale is called. If you just pass it a note, it can call getKey internally, which would simplify the function's usage to just:

getScale('a', 'minor');

But really, you don't need getKey, even internally. The first thing you do in getScale is call Object.keys(tonic)[0], so your code is essentially wrapping and unwrapping a letter:

getKey("a") => Object.keys({ "a": 1 })[0] => "a"

So getKey seems like an unnecessary detour.

You then "rotate" the chromatic scale, to start the right note, and remove some parts based on the type you want. The way you rotate the scale is perfectly fine, but there is also another way to pick the right notes without slicing and dicing the array. I'll get to that in a moment.

However, I'd propose a different notation for the sequence, namely one that lists the indices to keep, rather than which should be removed:

var sequences {
      major: [0, 2, 4, 5, 7, 9, 11],
      minor: [0, 2, 3, 5, 7, 8, 10]
    };

(Actually there's only one sequence: The minor one is simply the major one shifted 6 degrees. So defining both sequence manually isn't really necessary, but I'll leave any further refactoring to the reader.)

The reason for doing things like this is that it lines up well with making a list of labels for the notes - now all are lists of 7 elements:

var names = ["tonic", "superTonic", "mediant", "subDominant", "dominant", "subMediant", "leadingTone"];

(Incidentally, wikipedia informs me that the name "leading tone" is only used for the major scale, but on the minor scale it's named "subtonic". I'll leave that as an exercise to the reader.)

Now, for the output, I'd consider returning an array instead of an object. A scale is inherently ordered, so an array seems the most natural data type (objects in JavaScript do not guarantee consistent ordering of their properties). But of course you still want to be able to access notes by their name, e.g. scale.subdominant. JavaScript makes this possible with arrays too, though, because arrays are objects. So you can add the notes as indexed elements and as named properties.

Given this, if we use the reduce function to construct the array:

var scale = sequences[type].reduce(function (scale, noteIndex, i) {
  var note = rotatedScale[noteIndex], // get the note
      name = names[i];    // get the name
  scale.push(note);       // add the note as an array element
  scale[name] = note;     // add the note as a named property
  return scale;
}, []);

Now scale is an array with notes available both as ordered array elements, and by their names, e.g. scale[3] === scale.subDominant.

Finally, I mentioned a way to hit the right notes without slicing and dicing the chromatic scale. It involves knowing the offset of the input note, and the modulo operator (%):

var offset = chromaticScale.indexOf(key);
var scale = sequences[type].reduce(function (scale, noteIndex, i) {
  var index = (noteIndex + offset) % chromaticScale.length,
      note = chromaticScale[index],
      name = names[i];
  scale.push(note);
  scale[name] = note;
  return scale;
}, []);

The modulo operator makes the index value "wrap around".

All in all, with some input checking, you end up with a function like:

function getScale(key, type) {
  "use strict";

  // "constants"
  var chromaticScale = ['c', 'c#', 'd', 'Eb', 'e', 'f', 'f#', 'g', 'Ab', 'a', 'Bb', 'b'];
  var names = ["tonic", "superTonic", "mediant", "subDominant", "dominant", "subMediant", "leadingTone"];
  var sequences = {
            major: [0, 2, 4, 5, 7, 9, 11],
            minor: [0, 2, 3, 5, 7, 8, 10]
          };

  var offset = chromaticScale.indexOf(key);

  // return undefined if the key or type is unknown
  if(offset === -1 || !sequences[type]) return;

  return sequences[type].reduce(function (scale, noteIndex, i) {
    var index = (noteIndex + offset) % chromaticScale.length,
        note = chromaticScale[index],
        name = names[i];
    scale.push(note);
    scale[name] = note;
    return scale;
  }, []);
}

No need for an IIFE, as there's just one function.

Answer 2

Review

When analysing the notes of a scale, you lose information when only working with a predefined set of note names. C# is not the same as Db.

    //chromatic scale
    var chromatic = ['c', 'c#', 'd', 'Eb', 'e', 'f', 'f#', 'g', 'Ab', 'a', 'Bb', 'b'];

I would allow for any kind of combination of a degree with optional accidentals:

C# = Db = Ebbb = ..

Rather than using fixed scale names and magic mapping, I would opt for a generic approach that takes an identity value of a scale.

switch(type) {
        case "major":
        ..
        case "minor":
        ..

Scales can be identified by nominal id, interval pattern and pitch class set. I have opted to use pitch class set as identifier. These identifiers are explained by Ian Ring. For instance, the major scale can be identified as:

• nominal id: 2741
• pitch class set: [0,2,4,5,7,9,11]
• interval pattern: {2,2,1,2,2,2,1}

var leadingTone = {};

The leading tone is a misleading term, since it is used when it is a semi-tone below the tonic. But not all scales use this pattern, so an alternative is to use the superset name subTonic.