2
\$\begingroup\$

I am learning Node.js and I would like to see if I'm using it correctly:

var mysql = require('mysql');
var fs = require('fs');

var connectionProps = JSON.parse(fs.readFileSync('connection.json', 'utf8'));

var connection = mysql.createConnection({
  host: connectionProps.host,
  user: connectionProps.user,
  password: connectionProps.password,
  database: connectionProps.database
});

connection.connect();

function checkErr(err) {
  if (err) {
    connection.end();
    throw err;
  }
}

connection.query('SELECT * FROM TRANSACTION_TYPE', function(err, rows, fields) {
  checkErr(err);

  var typeNameToId = {};

  for (var i = 0; i < rows.length; i++) {
    typeNameToId[rows[i].NAME] = rows[i].ID;
  }

  fs.readFile(process.argv[2] + '.csv', 'utf8', function(err, content) {
    checkErr(err);

    var data = [];
    var lines = content.split(/\r\n?|\n/);

    for (var i = 0; i < lines.length; i++) {
      if (lines[i].trim() === '') {
        continue;
      }

      data.push(lines[i].split(','));
      var typeName = data[data.length - 1][2];
      data[data.length - 1][2] = typeNameToId[typeName];
      var dateTokens = process.argv[2].split('-');
      data[data.length - 1].push(new Date(dateTokens[0], dateTokens[1] - 1, dateTokens[2]));
    }

    connection.query(
      'INSERT INTO TRANSACTION (DESCRIPTION, AMOUNT, TRANSACTION_TYPE_ID, TRANSACTION_DATE) VALUES ?',
      [data],
      function(err) {
        connection.end();

        if (err) {
          throw err;
        }
      }
    );
  });
});
\$\endgroup\$
1
\$\begingroup\$

Connection

You get your connection with the standard mysql.createConnection method. OK, but you also use bare JSON sub-fields to initialise it. What if they're not there? You're going to get a ReferenceError whent the call tries to access it. I'd advise checking that and logging the mistake.

function createDatabaseConnection(properties) {
    if(properties.host && properties.user && properties.pass && properties.database) {
        return mysql.createConnection({
            host: properties.host,
            user: properties.user,
            password: properties.password,
            database: properties.database
        });
    }
    else {
        console.log("Could not retrieve all required details to connect to the database.");
    }
}

var connection = createDatabaseConnection(connectionProps);

Naming

There are a few - not many - places where your naming could be better.

  • checkErr - you could name this handleErrors, which would reflect slightly better what it does.
  • Table names - are your tables really called TRANSACTION_TYPES, all caps? If they are, OK, there's no standard, but if you're only doing it to match the SQL caps style, you shouldn't be.
  • typeNameToId- that reads like a method name. Perhaps call thistypeIds`?

Magic arguments

process.argv[2] - how do you know that what you want is always argument number 3? This might be a good point to look at implementing switches on the command line. You seem to be getting a file name here, perhaps you should use -f filename as the switch.

You've also got a number of array accesses which are done with magic numbers, though that's more common and not a massive issue.

Query errors

In your second query:

connection.query(
  'INSERT INTO TRANSACTION (DESCRIPTION, AMOUNT, TRANSACTION_TYPE_ID, TRANSACTION_DATE) VALUES ?',
  [data],
  function(err) {
    connection.end();

    if (err) {
      throw err;
    }
  }
);

You're redefining how you handle errors. If you simply pass a reference to checkErr (or whatever you've called it, by then) as the third argument instead of the function, you get consistent error handling. I notice your method definition here is different to that in checkErr - is there a reason you're closing the connection before checking if the error actually exists this time?

\$\endgroup\$
  • \$\begingroup\$ The reason that I close the connection before I check for an error is because I want to make sure that the connection will get closed. \$\endgroup\$ – SBel Jan 13 '16 at 18:04
  • \$\begingroup\$ @SBel so on the other hand: why do it after you check for the error in checkErr? \$\endgroup\$ – Åna Jan 13 '16 at 18:10
  • \$\begingroup\$ If I will call checkErr and there is no error then the connection won't get closed. \$\endgroup\$ – SBel Jan 13 '16 at 18:14
  • \$\begingroup\$ Fair enough. Just want to check that it's deliberate, rather than an error. \$\endgroup\$ – Åna Jan 13 '16 at 18:32

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.