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.
