Associating degrees with notes of scales

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--) {

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

StackExchange Code Review Q#116319, answer score: 10