Advent of Code - Day One I tried to write a foolproof function for the first part of the first problem on adventofcode, which asks for the count of ( characters minus the count of ) characters in a string.

The function intentionally ignores any other characters in the input string.


function findEndFloor(floors, upFloorSymbol, downFloorSymbol) {
  if (typeof floors !== 'string')
      throw 'parameter 1 is expected to be a string';
  if (typeof upFloorSymbol !== 'string')
      throw 'parameter 2 is expected to be a string';
  if (typeof downFloorSymbol !== 'string')
      throw 'parameter 3 is expected to be a string';

  var count = 0;
  var chars = floors.split('');
  for (var i = 0; i &lt;  chars.length; i++) {
      if(chars[i] === upFloorSymbol || chars[i] === downFloorSymbol)
          count += (chars[i] === upFloorSymbol ? 1 : -1);
  }

  return count;
}


Any feedback will be appreciated.
Alternate solution

First off, I want to provide an alternate solution.

Let's analyze the sample inputs and outputs provided:


  Input:

(()(()(

  
  Output:

3



Now, let's count how many of each character there is:


(: 5 occurrences
): 2 occurrences


Well, if ( is the "up" character and ) is the down character, subtracting their occurrences gives you the floor number.

Don't believe me? Here's another example:


  Input:

))(((((

  
  Output:

3




(: 5 occurrences
): 2 occurrences


5 - 2 = 3; again, the floor number is revealed.

Basically, it doesn't matter the order of characters; you are always going to end up on the same floor. Now your implementation can be simplified: count up the occurrences of each character and subtract them for your result.

var upOccurrences = floors.match(new RegExp(upFloorSymbol, "g")).length;
var downOccurrences = floors.match(new RegExp(downFloorSymbol, "g")).length;

return upOccurrences - downOccurrences;


Note that JavaScript may yell at you for using the above code depending on the character passed in. For example, if you use parentheses, you will get an error even if they are escaped in the argument list. Here is how to fix that:


  If you want to fix that regex error then just provide the parentheses
  as "\\(" and "\\)". That will change the regex to be /\(/g and /\)/g
  :)
  
  -tkellehe


An alternative solution for finding the amount of characters in a string would be this:

var occurrences = string.split(character).length - 1;


However, I am unsure if this is good practice in JavaScript (I know it's not in Java).



Review

Now, for a review on your code.



Splitting an array into the same array


var chars = floors.split('');



In JavaScript, strings are just like arrays with the exception that strings are immutable. Since you are not modifying this chars array as you loop through it, it is unnecessary to split it as you can just treat the string like an array.

Stop throwing strings at me! I can't trace them!


throw 'parameter 1 is expected to be a string';



This is bad practice. When an error is thrown like this, there is no way to search the stack trace for where the problem occurred. Also, you get a pretty weird console message:


  Uncaught parameter 1 is expected to be a string


That doesn't even make sense (grammatically).

You should be throwing an instance of an Error, like this:

throw new Error(...);


Now, when this error is thrown, a better message and a stack trace are provided. This makes tracking down where the error occurred a lot more easy:


  Uncaught Error: message (stack trace)


Even better, you can be more specific and JavaScript's built-in errors like TypeError, especially since you are dealing with the types of the arguments.



Half the size for twice the conditionals


  if(chars[i] === upFloorSymbol || chars[i] === downFloorSymbol)
      count += (chars[i] === upFloorSymbol ? 1 : -1);



I see what you are trying to do here. You are trying to make your code shorter by first squishing two conditionals into one, and then using a ternary.

This is a bad idea. With how you did it, you are now forced to create two more conditions (creating a total of 4, or something like that) to check when you really only need two. These extra conditions unnecessarily slow down your code.

Instead of combining two conditionals here:


  if(chars[i] === upFloorSymbol || chars[i] === downFloorSymbol)



Split it up like this:

if(chars[i] === upFloorSymbol) {
    count += 1;
} else if(chars[i] === downFloorSymbol) {
    count -= 1;
}


Now you don't need that extra ternary.



Braces keep your code straight

Always use braces with conditionals, loops, etc.