Return to Answer

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

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:

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:

(Actually there's only one sequence: The minor one is simply the major one shifted 6 degrees. So defining both 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 - all are lists of 7 elements:

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.

var scale = sequences[type].reduce(function (scale, noteIndex, i) {
  var note = rotatedScale[noteIndex],
      name = names[i];
  scale.push(note);
  scale[name] = note;
  return scale;
}, []);

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

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

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:

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:

(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:

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.

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;
}, []);

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

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

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:

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

(Actually there's only one sequence: The minor one is simply the major one shifted 6 degrees. So defining both 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 - all are lists of 7 elements:

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;
}, []);

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;
}, []);