API with revealing module pattern in Javascript

Question

I'm working on a project where I want to create a API that can be used to do some mathmatical calculations.

Here are the important aspects:

• I used the revealing module pattern
• I added method chaining (sort of like in JQuery)
• No Prototypes used (I don't see a advantage here)

My questions are the following:

1. Is it a mistake to not use prototypes ? I'm sort of thinking about the benefits I could gain by using them but I don't see any.
2. Is the error handling the correct way or should I do it differently ?
3. Does the code look viable and suitable for a API ?
4. Is there a other way of doing the method chaining ?

I'm also open for general suggestion to improve the code further which wasn't mentioned in the questions.

Edit: I haven't mentioned that I don't have a problem with using prototypes, I do understand the prototype inheritance and I also welcome any suggestion that is written with it.

Here is the code

function type(entity) {
  return Object.prototype.toString.call(entity).slice(8, -1);
}

function isNumber(entity) {
  return type(entity) === "Number";
}

let gMath = (function() {
  let currentNumber;
  let gotNumber = false;

  let number = function(number) {
    let numberType = type(number);

    if (isNumber(number) && !gotNumber) {
      currentNumber = gotNumber ? undefined : number;
      gotNumber = true;
    } else if (gotNumber) {
      console.log("Error! Cannot define 2 numbers. Only use .number() once or call .getNumber() to clear the current one.");
    } else if (type(number) === "String") {
      console.log(`This function only accepts Numbers as arguments. You passed in: "${numberType}"`);
    } else{
      console.log(`This function only accepts Numbers as arguments. You passed in: ${numberType}`);
    }
    return this;
  }

  let scatter = function(range) {
    currentNumber = Math.floor((currentNumber + (Math.random() * range * 2) - range) + 1);
    return this;
  }

  let randomize = function(number) {
    currentNumber = Math.floor(Math.random() * currentNumber);
    return this;
  }

  let atleast = function(number) {
    currentNumber = currentNumber < number ? number : currentNumber;
    return this;
  }

  let add = function(number) {
    currentNumber += number;
    return this;
  }

  let subtract = function(number) {
    currentNumber -= number;
    return this;
  }

  let show = function() {
    console.log(currentNumber);
    return this;
  }




  let getNumber = function() {
    gotNumber = false;
    return currentNumber;
  }

  let isCurrentNumber = function() {
    console.log("Currently the gotNumber variable is: " + gotNumber);
  }

  return {
    // chainable
    scatter,
    number,
    atleast,
    randomize,
    add,
    subtract,
    show,

    // non-chainable
    getNumber,
    isCurrentNumber
  }

})();

let a = gMath.number(10)
.scatter(50)
.getNumber();

console.log(a) // --> logs a Number between -40 and 60

Answer 1

Not using prototypes is completely fine. It can be advantageous at times to make use of them, but I agree that there is not much / any advantage to using them in this case.

If an error occurs, you should throw the error. Just logging it and moving on can result in some incredibly confusing behavior.

The biggest issue I see with using this code as an API is the inability use more than one number at once. If I want to use it to generate multiple numbers at once I can't. As a consumer of the library I would expect the following to work.

let a = gMath.number(50).scatter(50);
// Scatter twice by the same number
let b = a.scatter(a.getNumber()).scatter(a.getNumber()).getNumber();

There is another way to do method chaining that would solve the problem illustrated above. Instead of modifying the current state and returning this, return a new immutable gMath with the correct number set.

---

I'll point out a few potential problems / improvements with the current code, and then show how I would structure this api while keeping the usage the same.

1. There's no need to define isNumber. There is a built in method Number.isNaN that can check that with the additional benefit of handing NaN values (typeof NaN === "number")

2. There's no need to check gotNumber twice in the number function. This chain of if statements can be quickly improved if you throw errors for invalid inputs.

   if (Number.isNaN(number)) {
       throw Error('This function only accepts numbers');
   }
   if (gotNumber) {
       throw Error('Cannot define 2 numbers. Only use .number() once or call .getNumber() to clear the current one.');
   }
   currentNumber = number;
   gotNumber = true;
   

3. It looks like randomize might have a bug, it accepts a number but doesn't use it.

4. atleast can be simplified by using Math.max instead of the ternary.

5. None of your methods except number check if gotNumber is set. This leads to confusing behavior especially since number says calling getNumber clears the current number.

   gMath.number(10);
   gMath.getNumber(); // 10
   gMath.subtract(5);
   gMath.getNumber(); // 5
   

6. isCurrentNumber implies that the function returns a boolean. If you decide to keep with your current design of a single instance, this should probably be renamed to hasNumber and should return a boolean that the caller can log.

---

With this in mind, here is how I would write this module, keeping the same methods as the original for the most part. The one change is that instead of calling gMath.number(#) you call gMath(#) to create an instance.

function gMath(number) {
    if (Number.isNaN(number)) {
        throw Error('This function only accepts Numbers as arguments.');
    }

    let instance = {
        scatter(range) {
            let num = Math.floor((number + (Math.random() * range * 2) - range) + 1);
            return gMath(num);
        },

        randomize() {
            return gMath(Math.floor(Math.random() * number));
        },

        atleast(lowerBound) {
            return gMath(Math.max(lowerBound, number))
        },

        add(num) {
            return gMath(number + num);
        },

        subtract(num) {
            // Safer than using `this` as `this` can be changed 
            // by how the function is called.
            return instance.add(-num);
        },

        show() {
            console.log(number);

            return instance;
        },

        getNumber() {
            return number;
        },
    };

    return instance;
}

let a = gMath(10).scatter(40).getNumber()
console.log(a);

Answer 2

Checking numbers

The correct way to check if a value is a number:

function isNumber(value) {
  return !isNaN(parseFloat(value)) && isFinite(value);
}

The other uses of your custom type function can be eliminated.

Using prototypes

JavaScript is a prototypal language, prototypes are in its foundation. As such, I don't think it's a good idea to fight against prototypes. To get started on the right foot, I highly recommend Crockford's book, JavaScript: The Good Parts. See especially Chapter 5: Inheritance.

That being said, in this particular program, since you don't need inheritance, that means you don't need worry about prototypes either.

Useless conditional branches

These conditional branches look quite redundant:

} else if (type(number) === "String") {
  console.log(`This function only accepts Numbers as arguments. You passed in: "${numberType}"`);
} else{
  console.log(`This function only accepts Numbers as arguments. You passed in: ${numberType}`);

What's the point in printing a message slightly differently in case the type is string? I suggest to simply keep only the else, and drop the else if.

Your questions

Is the error handling the correct way or should I do it differently ?

No, it's not correct. There isn't real error handling here. Most functions that expect a number don't validate their input. So the error handling is simply missing.

Also, most functions expect to work with a number that was already given. But there are no guarantees that a number is given. For example nothing prevents you from calling gMath.scatter(50).

In the random function you check if the input is a number, and if it's the first time a number is given, but this is not a good approach. The thing is, the gMath object makes no sense without a number. And it can only accept a number once. Therefore, it will be better to make the number a required parameter when constructing the object

Does the code look viable and suitable for a API ?

No, for the reasons above. The purpose is also questionable. It would be better to come up with a good use case where such API, and method chaining is justified.