Is it good practice to make additional ckecks in code for internal use only?

Question

Is there sence to check all arguments and other conditions in code, if I know, I never pass wrong argument and it should work fine ( say I already made some checks outside of that code ).

Example:

this code:

/**
 * Applies function to all elements of array in specified
 * context.
 * If array is empty, returns null.
 */
MyNameSpace.foreach = function(arr, callback, context) {
   if (!(arr instanceof Array)) {
       throw new Error('first argument is not an array');
   }
   if (typeof callback !== 'function') {
       throw new Error('callback is not a function');
   }
   if (typeof arr.length === 'undefined') {
      throw new Error('Length undefined');
   }

   var con = typeof context === 'object' ? context : window;

   var i = arr.length;
   if ( i <= 0 ) {
       return null;
   }

   for ( j = 0; j < i; j++ ) {
        callback.call(con, j, arr[j]);
   }
}

could be:

MyNameSpace.foreach = function(arr, callback, context) {
   var con = typeof context === 'object' ? context : window;

   var i = arr.length;
   if ( i <= 0 ) {
       return null;
   }

   for ( j = 0; j < i; j++ ) {
        callback.call(con, j, arr[j]);
   }
}

score 3 · Answer 1 · answered Apr 09 '11 at 08:51

Absolutely it can be good practice. It can aid the stability of your app and help with debugging. Downsides are that it can clutter the code (making it harder to read) and that it can have an affect on performance (although for the majority of JavaScript apps performance won't be critical).

You have to take a view in each case. It really depends (1) how likely it is that the parameters will be incorrect and (2) how much damage it will do if they are.

score 1 · Answer 2 · answered Apr 09 '11 at 08:53

The general principle in software when it comes to testing inputs and outputs is: "Be flexible on inputs, strict on outputs." You should always have additional checks on your inputs so that your code doesn't break if your method gets something it doesn't expect.

score 1 · Answer 3 · answered Apr 09 '11 at 09:10

It's definitely good practice, as any code that is using your class or function may unknowingly pass you values which could cause your code to crash.
Eg: A programmer once accidentally sent a NULL pointer to my code which had to do a deep copy of the object that the pointer pointed to.
But since I was cloning the pointed to instance in the copy constructor's initialization list, nobody could figure out why the program crashed.
The programmer said that I should've kept a check for a NULL pointer, but eventually we decided to keep the check on his end since we needed to preserve the efficiency of copying through the initialization list, which was faster.

So you can decide on placing checks depending on:
1. Efficiency of using a copy-ctor initialization list.
2. How well you've documented the input parameters and limitations of your functions/classes.
3. Whether your code could crash/yeild security issues because of malicious inputs
4. What kind of exception handling you need for your software.

score 1 · Accepted Answer · answered Apr 09 '11 at 14:31

Although it may slow execution a little if you keep it enabled in production, you can use a type checking library, either in production or during debugging:

function TypeEnforce (func, ctx, config) {
    if (!(this instanceof TypeEnforce)) {
        return new TypeEnforce(func, ctx);
    }
    if (typeof func === 'string' && ctx) {
        this.funcString = func;
        func = ctx[func];
    }
    this.ctx = ctx;
    this.autoset = config && config.autoset;
    this.oldFunc = func;
}
TypeEnforce.prototype.argTypes = function (args) {
    var that = this;
    this.reqArgTypesFunc = function () {
        for (var i=0, argl = args.length; i < argl; i++) {
            if (
                (args[i] === 'Array' && Object.prototype.toString.call(arguments[i]) !== '[object Array]') ||
                (args[i] !== 'Array' && typeof arguments[i] !== args[i])
            ) {
                throw 'Argument ' + i + ' [' + arguments[i] + '] should be of type ' + args[i];
            }
        }
        that.oldFunc.apply(that.ctx, arguments);
    };
    if (this.autoset) {
        this.ctx[this.funcString] = this.reqArgTypesFunc;
    }
    return this;
};
TypeEnforce.prototype.props = function (props) {
    var that = this;
    this.reqPropsFunc = function () { 
        for (var p in props) {
            if (typeof arguments[p][props[p]] === 'undefined') {
                throw 'The property "' + props[p] + '" of argument no. ' + p + 
                        ' [' + arguments[p] + '] should not be undefined';
            }
        }
        var method = that.reqArgTypesFunc || that.oldFunc;
        method.apply(that.ctx, arguments);
    };
    if (this.autoset) {
        this.ctx[this.funcString] = this.reqPropsFunc;
    }
    return this;
};
TypeEnforce.prototype.get = function (props) {
    return this.reqPropsFunc || this.reqArgTypesFunc || this.oldFunc;
};

...which in your case you might use as follows:

if (!MyNameSpace) {
    var MyNameSpace = {};
}
MyNameSpace.foreach = function(arr, callback, context) {
   var con = typeof context === 'object' ? context : window;

   var i = arr.length;
   if ( i <= 0 ) {
       return null;
   }

   for ( j = 0; j < i; j++ ) {
        callback.call(con, j, arr[j]);
   }
};

TypeEnforce('foreach', MyNameSpace, {autoset:true}).
    argTypes(['Array', 'function']).
    props({0:'length'});

MyNameSpace.foreach(['a', 'b'], function (k, v) {
    alert(k+':'+v);
});

With this approach you can separate out the ugly type checking code, and even disable it if you only need it during testing. You could further add methods for argument or property value checking.

Is it good practice to make additional ckecks in code for internal use only?

4 Answers4