Return to Revisions

1 of 4

answered Jan 10, 2016 at 2:47

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 fall over). 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;
```
The the default case itself, the break isn't strictly necessary either, but this is like dotting the i's and crossing the t's.
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, but I might miss something basic or mix up the terminology.

First, I'd make it possible to actually call you 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. 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 the that in a moment.

However, I'd propose a different notation for the sequence, namely one that lists the indices to keep:

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

The reason for this is that it lines up well with making a list of labels for the notes - 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. 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],
      name = names[i];
  scale.push(note);
  scale[name] = note;
  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 = rotatedScale[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.

answered Jan 10, 2016 at 2:47

Flambino