4
\$\begingroup\$

I'm working on an old application written in JavaScript. It's absolutely littered with console.* messages some of which are useful for development purposes.

At the very least I'd like to switch these off when a production flag is set. I would also like to stub off any unsupported console methods (trace on IE for example). Note: Assume that we will always have ES5 support.

Concerns:

  1. I call into the initializer as soon as the constructor is instantiated. I like the idea of having a separate initializer method because it can be swapped out. I don't however like the idea that subsequent calls can be made. Any thoughts on this? I've seen it used a lot in Backbone's source.

  2. As you can see I've tried to keep the production flag separate from the general 'enabled' flag. When in production I noop all of the methods instead of wrapping them (or nooping them if unsupported). Perhaps there's a better way of catching all calls to console (like a jasmine style spy) in production?

  3. My hope with wrapping each supported method was to have a way of injecting pre-conditions/formatting before it's fired off. I also use this as an opportunity to check flags like enable. Is this over-contrived?

var Logger = function () {
    this._methods = [
        'assert', 'clear', 'count', 'debug', 'dir', 'dirxml',
        'error', 'exception', 'group', 'groupCollapsed', 'groupEnd',
        'info', 'log', 'markTimeline', 'profile', 'profiles',
        'profileEnd', 'show', 'table', 'time', 'timeEnd',
        'timeline', 'timelineEnd', 'timeStamp', 'trace', 'warn'
    ];

    this.initialize.apply(this, arguments);
};

var console = window.console, p = Logger.prototype;

p.initialize = function (options) {
    var noop = function () {};

    this.options = options || {};

    this._enabled = !this.options.production;

    this._methods.forEach(function (method) {
        if (this.options.production) {
            this[method] = noop;
        } else {
            this[method] = console[method] ? this._wrap(method) : noop;
        }
    }.bind(this));
};

p._wrap = function (method) {
    return function () {
        if (this._enabled) {
            return console[method].apply(console, arguments);
        }

        return false;
    };
};

p.enable = function () {
    if (!this.options.production) {
        this._enabled = true;
    }
};

p.disable = function () {
    this._enabled = false;
};
\$\endgroup\$
  • \$\begingroup\$ Did you tested if (function(){return this})().console and ''.search.constructor('return this').call().console return your fake console instance? \$\endgroup\$ – Ismael Miguel Nov 20 '14 at 12:57
  • \$\begingroup\$ I would wrap your whole code into this: (function(){window,undefined){'use strict';[...code here...]})(window); (on the function call, you can use one of my methods on the other comment to return the window object instead). This will protect your code from external variables and other things that people might try. \$\endgroup\$ – Ismael Miguel Nov 20 '14 at 13:03
4
\$\begingroup\$

Interesting question,

I spent a bit of time coding different approaches before writing this. Initially I just was going to point to my SO answer but then I saw you have some more requirements that are not met.

Concern #1

I dislike libraries where I have to initialize stuff outside of the constructor, the constructor should initialize an object so that it is usable! Why give me a half baked object that I cannot use? The idea of being able to swap out a separate initializer is typical YAGNI.

Concern #2

In my mind, the caller should decide whether to enable or disable (possibly based on a production flag or not), for your code to take care of both concerns independantly seems overkill.

Concern #3

It seems over-contrived to me ;)

I did like though the idea of fool-proofing all console methods for an ancient legacy code base, and kept that with 2 simple methods to turn of or on all output in my approach:

function logController(){

  var methods = [
    'assert', 'clear', 'count', 'debug', 'dir', 'dirxml',
    'error', 'exception', 'group', 'groupCollapsed', 'groupEnd',
    'info', 'log', 'markTimeline', 'profile', 'profiles',
    'profileEnd', 'show', 'table', 'time', 'timeEnd',
    'timeline', 'timelineEnd', 'timeStamp', 'trace', 'warn'
  ];
  //The one acceptable time we can pollute the global namespace ;]
  console = console || {};

  function stop(){
    var _method;
    methods.forEach( function (method){
      _method = '_' + method;
      console[_method] = console[_method] || console[method];  
      console[ method] = function noLog(){};
    });
  }

  function start(){
    var _method;
    methods.forEach( function (method){
      _method = '_' + method;
      console[method] = console[_method] || console[method] || function noLog(){};
    });    
  }
  start();

  return {
    start: start,
    stop: stop
  };
}

flow = logController();
console.log( 'Normal logging, next is 123' );
console.log(  123 );
console.log( 'Start logging (which is default state), next is 123' );
flow.start();
console.log(  123 );
console.log( 'Stop logging (which is default state), next is "start logging again"' );
flow.stop();
console.log(  123 );
flow.start();
console.log( 'Started logging (which is default state), next is 123' );
console.log(  123 );

I will keep the exercise to make this work with AMD/UMD in your capable hands ;)

\$\endgroup\$
  • \$\begingroup\$ I think I understand why you've used an underscore prefix to the method names but sticking to the convention of '_' meaning 'dont use this externally' I've opted to leave that out. Am I missing something? In any case I've updated my code. Thanks again. \$\endgroup\$ – backdesk Nov 24 '14 at 9:15
  • \$\begingroup\$ In this case, the _log etc. functions were indeed not to be used, since those functions cannot be turned off. as for the variable _method, I could not find a better name. \$\endgroup\$ – konijn Nov 24 '14 at 13:03
1
\$\begingroup\$

Here's my code based upon the review provided by konijn. I've taken the liberty of packaging my code up into a UMD.

(function (root, factory) {
    if (typeof define === 'function' && define.amd) {
        define(function () {
            return (root.Logger = factory());
        });
    } else if (typeof exports === 'object') {
        module.exports = factory();
    } else {
        root.Logger = factory();
    }
}(this, function () {

Added in strict mode as suggested by reviewer.

    'use strict';

As you can see I've removed the use of '.initialize.apply' in favour of doing init within the constructor:

    var Logger = function (options) {
        this._methods = [
            'assert', 'clear', 'count', 'debug', 'dir', 'dirxml', 
            'error', 'exception', 'group', 'groupCollapsed', 'groupEnd', 
            'info', 'log', 'markTimeline', 'profile', 'profiles', 
            'profileEnd', 'show', 'table', 'time', 'timeEnd', 
            'timeline', 'timelineEnd', 'timeStamp', 'trace', 'warn'
        ]; 

        if(options && options.enabled === false) {
            this.disable();
        } else {
            this.enable();
        }
    };

    var noop = function () { return false; }, console = window.console || {};   

Reduced the code down to 2 simpler methods that either enable or disable the supported console methods by nooping or proxying them. As suggested by the reviewer I've ditched the production flag since I can now pass in an option to enable or disable the console on init.

    Logger.prototype.enable = function () {
      this._methods.forEach(function (method) {
            this[method] = console[method] ? console[method] : noop;
      }.bind(this));
    };

    Logger.prototype.disable = function () {
      this._methods.forEach(function (method) {
            this[method] = noop;
      }.bind(this));
    };

    return Logger;
}));
\$\endgroup\$
  • \$\begingroup\$ One small amendment. Ensure that your proxied console methods are run in the scope of console. \$\endgroup\$ – backdesk Nov 25 '14 at 9:46
  • \$\begingroup\$ this[method] = console[method] ? console[method].bind(console) : noop; \$\endgroup\$ – backdesk Nov 25 '14 at 9:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.