Utilities for various builtins #1

Question

This is an iterative review. The next iteration is located here: Utilities for Various Builtins #2 This code puts several builtins (And a few non-builtins) into the base classes.

  const XtraUtils = {};
  class Utility {
    constructor(ofWhat){
      this.utils = {};
      this.aidsIn = ofWhat;
    }
    activate() {
      for (var x in this.utils) {
        window[this.aidsIn].prototype[x] = this.utils[x];
      }
    }
    addUtil(util, name) {
      this.utils[name] = util;
    }
  };
  XtraUtils.Number = new Utility("Number");
  XtraUtils.String = new Utility("String");
  XtraUtils.Array = new Utility("Array");
  XtraUtils.Number.addUtil(function(in_min, in_max, out_min, out_max) {
    return (this - in_min) * (out_max - out_min) / (in_max - in_min) + out_min;
  }, "map");
  XtraUtils.Number.addUtil(function(inputBase = 10, outputBase = 10){parseInt(this, inputBase).toString(outputBase)}, "toBaseN");
  XtraUtils.Number.addUtil(function(n){
    return Math.pow(this, n);
  }, "toTheNthPower");
  XtraUtils.Number.addUtil(function(){
    return Math.ceil(this);
  }, "ceil");
  XtraUtils.Number.addUtil(function(){
    return Math.floor(this);
  }, "floor");
  XtraUtils.Number.addUtil(function() {
    return Math.sqrt(this);
  }, "sqrt");
  XtraUtils.Array.addUtil(function() {
    const i = this.map(function(item) {
      return typeof item == "number";
    });
    const isAllNums = i.indexOf(false) == -1;
    if (!isAllNums) {
      throw "The array must be all numbers.";
      return;
    };
    let max = 0;
    let isFirst = true;
    this.forEach(function(item) {
      if (isFirst || item > max) {
        max = item;
        isFirst = false;
      };
    });
    return max;
  }, "max");
  XtraUtils.Array.addUtil(function() {
    const i = this.map(function(item) {
      return typeof item == "number";
    });
    const isAllNums = i.indexOf(false) == -1;
    if (!isAllNums) {
      throw "The array must be all numbers.";
      return;
    };
    let min = 0;
    let isFirst = true;
    this.forEach(function(item) {
      if (isFirst || item < min) {
        min = item;
        isFirst = false;
      };
    });
    return min;
  }, "min");
  XtraUtils.Array.addUtil(function() {
    let i = this.map(function(item) {
      return typeof item == "number";
    });
    const isAllNums = i.indexOf(false) == -1;
    if (!isAllNums) {
      throw "The array must be all numbers.";
      return;
    };
    let total = 0;
    var isFirst = true;
    this.forEach(function(item) {
      total += item;
    });
    const mean = total / this.length;
    return mean;
  }, "mean");

Feel free to use the utils if you like.

Answer 1

For the most part, this looks good to me. Your api is clear and I really like that you have to activate the utility before the prototype is modified. However, there's always room for improvement :)

1. You provide the Utility.activate function, it would be useful to also have a Utility.deactivate function.

2. addUtil taking name as the second parameter seems odd to me. On a longer utility method like min, max or mean it was not immediately obvious to me what the function did - I had to skip to the end or figure it out from the code.

3. Be consistent with your naming scheme. In most of the code you use camelCase, but in the parameters for Number.map you use snake_case.

4. Why is the inputBase the first parameter in toBaseN? Presumably, if I have a number it is already in base 10. This function will also have issues if you use 0.0000001, plug 0.0000001.toString() into your console and you will get 1e-7 instead of what you expected.

5. toTheNthPower is just pow on the Math object. I would use the same convention here.

6. Don't throw strings. Throw a new Error(str) or just Error(str). This makes it much easier for error tracking systems to give you helpful stack traces.

7. Returning 0 as a signal number if the array is empty is not a good idea. 0 could exist in an array. I'd recommend using the convention of Math.max and Math.min and returning -Infinity and Infinity respectively.

8. Mapping over the array to create an array of booleans is very inefficient. If I create an array with ['hi'].concat([1].repeat(1e9)) you still go through every single item in the array. Instead of doing this, bail out as soon as you hit a failing condition. [].every and [].some can be of help here. const isAllNums = this.every(item => typeof item == 'number')

9. Speaking of Math.max and Math.min, you should use them! You can use Math.max.apply(Math, this) or if your environment supports the spread operator, you can just use Math.max(...this). If desired, you can check if this returns NaN and throw an error to maintain the current API instead of returning NaN for arrays with non-numbers.

10. Don't use for..in loops to loop over keys of objects. Either use for (const key of Object.keys(this.utils)) or Object.keys(this.utils).forEach(. This will help you avoid problems with built in values like toString.

11. In this case, it is probably a better fit to use a Map instead of a plain object to hold the utils.

12. What happens if you want to use these utils in Node? They break - window is not defined. This might not be a consideration, but quite a lot of code runs on Node now.

13. What happens if you want to define utilities for an object which is not exposed on the global? Instead of passing a string into the constructor of Utility, I'd recommend passing in an object instead. new Utility(Array)

14. Instead of looping through the array multiple times for mean, I'd recommend checking each element of the array as you add it to the sum. Maintaining your current API, here's how I'd implement that function.

    function mean(arr) {
      return arr.reduce((total, item) => {
        if (typeof item !== 'number') {
          throw new Error("The array must be all numbers.")
        }
        return total + item
      }, 0) / arr.length
    }