2014.08.05

Type check is a code smell

Will try to be brief, so it might not be clear unless you are experienced, will add more references at the end to make things clearer and maybe revise this subject later… - Time is a very limited resource nowadays.

How I got into this conclusion?

JavaScript is a dynamic (and confusing), language, the way inheritance and typeof and instanceof works is not clear to most developers:

// JSWTF
typeof [1, 2] // 'object'
typeof null // 'object'
typeof 'foo' // 'string'
typeof new String('foo') // 'object'
'foo' instanceof String // false
new String('foo') instanceof String // true
new String('foo') instanceof Object // true

And things gets way more complicated when you have objects that come from different documents/frames (they are created using different constructors, so instanceof doesn’t work)… So because of that I started to avoid typeof and instanceof and ended up realizing how cleaner the code becomes when you avoid type checking.

PS: I know that we can use Object.prototype.toString.call() for type checking, I’m mainly saying that most type checks are a bad idea, doesn’t matter what is the implementation.

Type checks are a red flag!

99% of the time you see a type check it is telling you that the function should be split into multiple functions that handle a single type or that you should rely on JavaScript dynamic nature and abuse duck-typing and type casting/coercion.

Let’s see some overly simplified examples:

typeof

// ANTI-PATTERN!!
function sum(a, b) {
  if (typeof a === 'string') {
    a = parseFloat(a);
  }
  if (typeof b === 'string') {
    b = parseFloat(b);
  }
  return a + b;
}

Could be written as:

function sum(a, b) {
  // type casting to number because `"2" + 2 === "22"`
  return Number(a) + Number(b);
}

Which is easier to understand and does basically the same thing. Another common example that I see all the time is people using typeof checks to make sure the value can be used:

// yes, I've seen a lot of code like this before :/
function doStuff(options) {
  var amount;
  // warning: this might not work as expected since `typeof null === "object"`
  if (typeof options === 'object') {
    amount = options.amount;
  }
  // ... later inside same function
  if (typeof amount === 'number') {
    doSomethingElse(amount);
  }
}

First thing, this is redundant.. Let’s assume whoever is calling the method knows which kind of data he should pass; if he passes a value that can’t be used properly, then it will probably throw an error (automatically) anyway, so no point in being extra cautions… You might be making it harder to understand why something is not working as expected; silent failures are the worse.

Checking for != null is usually enough for cases like this and increases the flexibility. Code becomes less verbose, easier to understand and also more flexible like this:

function doStuff(options) {
  var amount = options && options.amount;
  // will be true for any value that is not `null` or `undefined`, that allows
  // us to pass `0` as well
  if (amount != null) {
    doSomethingElse(amount);
  }
}

instanceof

instanceof for me is the biggest offender, it forces you to sub-class a given type just to be able to pass the object into a function, and as we know, sub-classing in JavaScript can be a tricky thing, specially when you need to subclass a native object (eg. Array) or share objects between multiple documents/frames. If you use instanceof you are making your code less flexible for no real gain while also making it harder to understand and reuse. - mo’ conditionals, mo’ problems.

// ANTI-PATTERN!!
function doStuff(obj) {
  if (obj instanceof Foo) {
    obj.foo();
  } else if (obj instanceof Bar) {
    obj.bar();
  }
  // ... more stuff here
}

What we really want to know is if the given object contains the method we care about:

// better, but still wrong!
function doStuff(obj) {
  if (obj.foo) {
    obj.foo();
  } else if (obj.bar) {
    obj.bar();
  }
  // ... more stuff here
}

But to be honest, the real problem is that we have a single function that accepts multiple types of inputs, this is an anti-pattern by itself. It should really be split into 3 functions:

function doFoo(obj) {
  obj.foo();
  sharedDoStuff(obj);
}

function doBar(obj) {
  obj.bar();
  sharedDoStuff(obj);
}

function sharedDoStuff(obj) {
  // ...
}

That way you reduce the amount of conditionals and makes it clear what kind of action the method executes and can describe which data it expects as input. It also allows you to pass any type of object as long as it implements the proper API, which makes it easier to test and increase the chance of reuse.

Another solution is to make sure all the objects passed to the method implements the same method, which in my opinion is the best solution in most cases. Consistent APIs avoids mistakes and makes code easier to understand.

function doStuff(obj) {
  obj.execute();
  // ... more stuff here
}

Conclusion

So every time you see yourself doing a type check or adding lots of if, else, switch to your codebase stop and reflect if that is really the best option… Big messes starts small and the person who will end up maintaining your code might have a hard time understanding all the conditionals and might also have his productivity affected by all the unnecessary type checks (bureaucracy kills productivity).

“Everything should be made as simple as possible, but not simpler.”

PS: sorry for the bad examples and for not explaining it better, but was trying to write this during lunch and did not have time to think of anything better or to revise it.

Further reference


Comments

typeof NaN //"number"

Thanks for your blog, I agree with you that type checking clutters your code and makes it less easy to reuse, and has a code smell. But at the same time a lack of type checking leaves developers out in the dark with unhelpful error messages like "Error: cannot call undefined of foo", thrown by some deeply nested sub-module. I'm struggling with finding a good solution in this regard.

Most important for me when writing a function is not that the code of the function itself is a simple as possible, but that the use of it is as simple as possible, and that the user of the function gets a meaningful error messages when using the function wrongly. I would love to achieve this without type checking (preferably the ducktype way), but often see no alternative.

Unfortunately, a type casting solution like your example Number(a) + Number(b) only works for simple cases, but is not trivial when working with more complicated types/functions/arguments.

Splitting a function doStuff into doFoo and doBar only offloads your type problem to the user of your function. It does not solve the problem, and it clutters your API terribly.

The following code will probably terribly hurt your eye, it's full of type checking: https://github.com/josdejong/mathjs/blob/master/lib/function/arithmetic/add.js This is a function add(a, b), which accepts the following input types: Number, BigNumber, Boolean, Complex, Unit, String, Array, and Matrix. I'm far from happy with the current solution (though it works great from a user point of view). I'm curious what your approach would be here. I doubt whether you would split this single function into 8*8=64 functions each handling a different combination of argument types, dumping the type problem at the users side. I myself was thinking about writing some kind of "function router" to do this type checking and casting for me so I can just provide a small number of "sub" functions each handling a single input type. (Basically what compilers of strict programming languages automatically do for you...) However, this too has code smell.

Jos I agree that there are good use cases for type-checking (64 functions is not practical), I just think that it should be used mostly by libraries and helper functions (never scattered across your whole application code).

I also use type-checking in some functions on MOUT since it's the only way to solve some problems. I explained a little better how I use some key helper functions to reduce the amount of type checks to a minimum (specially for error handling) on this gist. Maybe I would use the same approach (always convert the values into something that I know how to sum) or let the consumer do the conversion before calling the method `add(toNumber(someWeirdvalue), someNumber)` (which is worse for the user)

Cheers.

Thanks for your feedback Miller. I agree, type-checking should not be scattered across an application and should be used only when really necessary (typically at the API layer of a module I think).

What about using something like fantasy-environment to create polymorphic functions which type checks in a clean manner? E.g.,

module.exports = environment()

// no type checking .property('myFunctionName', myFunction)

// type checking on parameters with single parameter .method( 'myOtherFunctionName', _.isString, myOtherFunction )

Cesar William

I think polymorphism is not simple to understand and implement in JavaScript (mainly when you don't have the clear definition of primitive and complex objects), but with a good algorithm analysis and separation of concern you can simplify it.

Do you agree that makes more sense use tools like flow or typescript as static type check? Or isn't a real solution?

Leave a Comment

Please post only comments that will add value to the content of this page. Please read the about page to understand the objective of this blog and the way I think about it. Thanks.

Comments are parsed as Markdown and you can use basic HTML tags (a, blockquote, cite, code, del, em, strong) but some code may be striped if not escaped (specially PHP and HTML tags that aren't on the list). Line and paragraph breaks automatic. Code blocks should be indented with 4 spaces.