current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

I'm new to JavaScript, and in Java I probably would do this as a singleton. I need something that I can call from anywhere and instantiate only once.

This is what I have done, but I need some advice to write it in the right way in this new prototype based and functional JavaScript.

var winston = require('winston');

var genericLogger = null,
    dbLogger = null,
    viewLogger = null,
    networkLogger = null;

function initLogger() {
    console.log('logger.js > initLogger');

    genericLogger = new (winston.Logger)({
        transports: [
          new (winston.transports.Console)(),
          new (winston.transports.File)({ filename: './log/error.log' })
        ]
    });

    dbLogger = new (winston.Logger)({
        transports: [
          new (winston.transports.Console)(),
          new (winston.transports.File)({ filename: './log/db.log' })
        ]
    });

    viewLogger = new (winston.Logger)({
        transports: [
          new (winston.transports.Console)(),
          new (winston.transports.File)({ filename: './log/view.log' })
        ]
    });

    networkLogger = new (winston.Logger)({
        transports: [
          new (winston.transports.Console)(),
          new (winston.transports.File)({ filename: './log/network.log' })
        ]
    });
}

initLogger();
var Logger = {}; 

Logger.error = function(n) {
  genericLogger.log('error', n);
};

Logger.info = function(n) {
  genericLogger.log('info', n);
};

Logger.db = function(t, n) {
    if(n) {
        dbLogger.log(t, n);
    } else {
        dbLogger.info(t);
    }
};

Logger.view = function(t, n) {
    if(n) {
        viewLogger.log(t, n);
    } else {
        viewLogger.info(t);
    }
};

Logger.network = function(t, n) {
    if(n) {
        networkLogger.log(t, n);
    } else {
        networkLogger.info(t);
    }
};

module.exports = Logger;

I can call it from anywhere with require, and I expose only few public methods with module.exports and due to the require mechanism there is only one instance.

Is there a cleaner way to do it? More OOP? More functional?

share|improve this question
up vote 1 down vote

Interesting question,

You have a ton of repeating code, so don't hesitate to write more helper functions so that this:

genericLogger = new (winston.Logger)({
    transports: [
      new (winston.transports.Console)(),
      new (winston.transports.File)({ filename: './log/error.log' })
    ]
});

dbLogger = new (winston.Logger)({
    transports: [
      new (winston.transports.Console)(),
      new (winston.transports.File)({ filename: './log/db.log' })
    ]
});

viewLogger = new (winston.Logger)({
    transports: [
      new (winston.transports.Console)(),
      new (winston.transports.File)({ filename: './log/view.log' })
    ]
});

networkLogger = new (winston.Logger)({
    transports: [
      new (winston.transports.Console)(),
      new (winston.transports.File)({ filename: './log/network.log' })
    ]
});

could be:

function createLogger( filename ){
  return new (winston.Logger)({
    transports: [
      new (winston.transports.Console)(),
      new (winston.transports.File)({ filename: filename })
    ]
  });
}
genericLogger  = createLogger( './log/error.log' );
dbLogger       = createLogger( './log/db.log' );
viewLogger     = createLogger( './log/view.log' );
networkLogger  = createLogger( './log/network.log' );

I did not test this code, but you get the gist.

Furthermore, in this code, I have no idea what t and n are:

Logger.db = function(t, n) {
    if(n) {
        dbLogger.log(t, n);
    } else {
        dbLogger.info(t);
    }
};

Logger.view = function(t, n) {
    if(n) {
        viewLogger.log(t, n);
    } else {
        viewLogger.info(t);
    }
};

Here as well, you could use a helper function:

Logger.loggerHelper = function( o , t , n ){
  if(n){
    o.log(t,n)  
  } else {
    o.log(t)
  }
}

Logger.db = function(t, n) {
  loggerHelper( dbLogger , t , n );
};

Logger.view = function(t, n) {
  loggerHelper( viewLogger , t , n );
};

After a while I would get irritated by the above as well, and write something like this:

var winston = require('winston');

var Logger = {};
var loggers = {
  generic : './log/error.log',
  db : './log/db.log',
  view : './log/view.log',
  network : './log/network.log'
};

for( logger in loggers){

  Logger['_' + logger] =  new (winston.Logger)({
    transports: [
      new (winston.transports.Console)(),
      new (winston.transports.File)({ filename: loggers[logger] })
    ]
  });

  Logger[logger] = function(t,n ){
    if(n) {
        Logger['_' + logger].log(t, n);
    } else {
        Logger['_' + logger].info(t);
    }  
  }

}

Logger.error = function(n) {
  Logger['_generic'].log('error', n);
};

Logger.info = function(n) {
  Logger['_generic'].log('info', n);
};

module.exports = Logger;

loggers contains the link between the method and the file name, so it becomes now very easy to add/remove/change loggers. This approach does expose a lot more (all) the functionality in Logger, but I think that would work out.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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