1
\$\begingroup\$

I am writing a trivial continuous integration tool using node.js to learn the platform. So far it can download a project from Github, and unzip it into a random folder in a predefined dropfolder. But I'm pretty inexperienced in pure JavaScript and loosely typed languages in general and the code that I write looks very chaotic to me. Below are the relevant parts of my tiny application (full source on GitHub):

config.json

{
  "port": "8081",
  "dropDownFolder": "E:\\dropdown",
  "loggers": [
    {
      "type": "consoleLogger"
    },
    {
      "type": "consoleLogger"
    },
    {
      "type": "fileLogger",
      "logFile": "log.log",
      "errorFile": "error.log"
    }
  ],
  "builds": [
    {
      "name": "SimpleCI",
      "urlToListen": "/simpleci",
      "copyTo": "E:\\dropdown",
      "dataSource": {
        "sourceType": "github",
        "user": "andreysarafanov",
        "repository": "SimpleCI",
        "branch": "master"
      }
    }
  ]
}

Logging/loggerBuilder.js

var config = require("./../config.json");
var FileLogger = require('./fileLogger');
var ConsoleLogger = require('./consoleLogger');
var _ = require('underscore');

module.exports = function buildLogger(callback) {
  //console.log("start building loggers " + config.logger);
  var loggers = [];
  _.each(config.loggers, function(logger) {
    if (logger.type === "fileLogger") {
      loggers.push(new FileLogger(logger.logFile, logger.errorFile));
    }
    else if (logger.type === "consoleLogger") {
      loggers.push(new ConsoleLogger());
    }
    else {
      callback("Logger type \"" + logger.type + "\" is not supported.");
    }
  });

  return {
    log: function log(message) {
      _.each(loggers, function (logger) {
        logger.log(message);
      });
    },
    error: function error(message) {
      _.each(loggers, function (logger) {
        logger.error(message);
      });
    }
  };
}

Logging/fileLogger.js

var fs = require('fs');
var endOfLine = require('os').EOL;
module.exports = function fileLogger(logFile, errorFile) {
  return {
    log: function log(message) {
      fs.appendFile(logFile, buildString("debug", message));
    },
    error: function error(message) {
      fs.appendFile(logFile, buildString("error", message));
    }
  };
}

function buildString(type, message) {
  var time = new Date();
  return type.toUpperCase() + ": [" + time.getYear() + "." 
    + time.getMonth() + "." + time.getDate() + "] - " + message + endOfLine;
}

Logging/consoleLogger.js

module.exports = function consoleLogger() {
  return {
    log: function log(message) {
      console.log(buildString(message));
    },
    error: function error(message) {
      console.error(buildString(message));
    }
  };
}

function buildString(message) {
  var time = new Date();
  return "[" + time.getYear() + "." + time.getMonth() + "." 
    + time.getDate() + "] - " + message;
}

index.js

var http = require("http");
var express = require("express");
var _ = require("underscore");
var config = require('./config.json');
var dataLoader = require("./DataLoaders/dataLoader.js")
var uuid = require('node-uuid');
var LoggerBuilder = require('./Logging/loggerBuilder');

var app = express();

var logger = LoggerBuilder(function(err) {
  if (err) console.error(err);
})

_.each(config.builds, function(build) {
  app.get(build.urlToListen, function(req, res) {
    console.log("A request for build " + build.name + " has been received");
    var id = uuid.v4();
    dataLoader.downLoadDataForBuild(build.dataSource, id, function(err) {
      if (err) {
        logger.error(err);
        res.send("Error: " + err);
      }
      else {
        logger.log("downloaded data for build " + build.name + " to subdirectory " + id);
        res.send("Success");
      }
    })
  })
});

app.listen(config.port);

I can configure multiple loggers in config.json and the output will be written to all of them (in this case - twice to the console and to the file). The code seems to work as expected, but I ended up relying on string constants("fileLogger" and "consoleLogger" for logger type, the names of the fields in config.json), and the code that initializes logger objects looks boilerplate to me. How would you refactor the code to make it more ordered and less error-prone?

\$\endgroup\$
1
\$\begingroup\$

Your logger configuration doesn't seem bad. It's a little verbose, but it is open to further extensions if those ever come along. I don't know your requirements, but if you're only ever going to have one file logger and optionally a console logger, you could change your list into an object like so:

loggers: {
  "console": true,
  "file": {"logFile": "out.log", "errorFile": "err.log"}
}

But having a list is just fine. Not knowing anything else about your requirements, I would leave it the way it is.

The thing that really sticks out to me in your code is the use of a callback as a parameter in loggerBuilder.js. Since this function is not asyncronous, I would ditch the callback and either log the error directly to the console or just throw the error and let it break the build. If you really want to provide a mechanism for reporting configuration errors to the user, then rename "callback" to something akin to "errorHandler". The use of the name "callback" usually signifies a function that will be executed once and only once, will be executed when the function has completed, and will not return a completed value but instead pass the value as an argument to the function.

Also, I find it surprising that you do not log the time to the output of the file or console logger. Not a big deal of cource, it just stuck out.

\$\endgroup\$
  • \$\begingroup\$ Danny, thanks! I don't have any requirements at all and just thought that having a list of loggers in config is the most extensible of the simple solutions. Guess I'll refactor the callback in loggerbuilder.js. Do I get it right that code like if (logger.type === "fileLogger") is pretty much normal in javascript? For example, I'd definitely call it smelly and eagerly refactor it into a enum or at least some constant string in C#. \$\endgroup\$ – AndreySarafanov Dec 3 '15 at 15:07
  • 1
    \$\begingroup\$ @AndreySarafanov, You're absolutely correct, a check like that is normal in javascript. If your node version supports it, you could define a const for the different types, but it would still be a string in the end. And I'm with you, if I was in java land I would make this into an enum in a heartbeat. :) \$\endgroup\$ – Danny Kirchmeier Dec 3 '15 at 16:34

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.