10
\$\begingroup\$

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.

\$\endgroup\$
  • 5
    \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$ – SirPython Jan 9 '16 at 23:05
  • 1
    \$\begingroup\$ Slightly off topic, but very relevant since this is one of your first coding experiences (very good job btw!). As a demonstration of the cool and powerful things you can do now with code: Since you can work with Javascript, I'd highly recommend looking into the WebMIDI API. I'm assuming you're a musician (and you may have a MIDI input device). This API could allow you to do really cool integrations with that device. For example, you could have your code produce a scale and then play it with the WebMIDI API, or recognize a scale from the notes played on your midi piano. Good luck & have fun! \$\endgroup\$ – Bailey Parker Jan 11 '16 at 17:41
  • \$\begingroup\$ I appreciate that! I'll look into it. \$\endgroup\$ – isosceles Jan 12 '16 at 22:50
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ While i agree with most of your comments, using an IIFE and the module pattern for such a small project, and a beginner, seems to me like using a nail to kill a bee. Here there are two function, so you save... one global... The (gifted indeed!) O.P. has many things to learn and time before its project requires such patterns. \$\endgroup\$ – GameAlchemist Jan 10 '16 at 12:30
  • \$\begingroup\$ @GameAlchemist Well, at the end of my answer you'll see I propose a solution that doesn't actually use an IIFE either, because there's no need for it anymore. But I'm not going to say that OP is wrong to use an IIFE either. Even if it was just one function. And using the module pattern namespaces things, aiding maintainability - again even if it's just a couple of functions. Point is, OP did the right thing, earlier than other beginners might. I'm not going to say that's wrong. Even the smallest projects benefit from good structure, and good habits should be formed early on. \$\endgroup\$ – Flambino Jan 10 '16 at 20:15
  • \$\begingroup\$ Thank YOU! sincerely for your suggestions, feedback and everything you took the time to highlight, I was hesitant initially, but I'm glad I put this up. This will be a great little exercise to work with on refactoring and building up.. your suggested changes are elegant and your use of reduce, poetic. I've read of reduce and it's versatility and I'm excited you used it here-I can now play with and make sense of it using an example I'm familiar with. It will take sometime to internalize it's use but I'm happy you added it here. \$\endgroup\$ – isosceles Jan 10 '16 at 21:28
  • \$\begingroup\$ Flambino, can I ask you to go into a little more detail on how you've set up scale to work with reduce? I wrote a massive detailed question that went over the limit by several thousand chars, so I can't paste any of it here.. here's the short and abrupt version of it: Line 4: We push each note into scale? Scale is a variable we'll set our final result to (an []) but how do we push things into a var. that's sitting outside the callback and while the callback is firing and then return it from within reduce? If this is too vague, I can email you my initial question : ) \$\endgroup\$ – isosceles Jan 15 '16 at 0:15
  • \$\begingroup\$ @isosceles Can't give a detailed explanation here and now (work + travelling), but it's down to how reduce works. I've made it a bit confusing by naming an argument scale, and storing the return value in a variable also called scale - try calling the argument x and use x.push(…) and x[…] instead. Point is, for each element in sequences[type] stuff is pushed to an array that's then passed on to the next iteration and so on. After the final iteration the array's passed "back" to the caller... yeah, hard to explain. Read up on reduce and play around with it is my best suggestion. \$\endgroup\$ – Flambino Jan 15 '16 at 0:33
2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ – Toby Speight May 22 at 17:30
  • \$\begingroup\$ @TobySpeight I have removed my alternative solution. I will ask a follow-up question about it instead. \$\endgroup\$ – dfhwze Aug 21 at 16:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.