Node.js DataBase class for adodb databases

Question

A while ago I was working on a multipurpose database class (an es6 javascript class) that works with any database supporting adodb.

This class is a wrapper on the "node-adodb" npm library. This database class has four main methods, all of which process some options, passed in as parameters, into sql, which is then run on the database.

I am particularly worried about my processing of the options, as I have little experience in that field.

I have been programming for about 2 years or so and am looking for any criticisms/feedback.

Here is my code:

'use strict'

/**
 * @file A file that contains the "dataBase" class.
 */
const debug = require('debug')('Data-comms:dataBase')
const adodb = require('node-adodb')
adodb.debug = true

/**
 * @constructor
 * @param {string} connection - The connection string to the database.
 * @param {boolean} X64  - Whether or not you are using the 64 bit version.
 * @name dataBase
 * @description A class for: Connecting to, Querying, etc, adodb databases.
 */

exports.dataBase = class DataBase {
  constructor(connection, X64) {
    this.connectString = connection
    this.X64 = X64
    this.connection = adodb.open(connection, X64)
    debug('Connection opened:', this.connection)
    this._this = this
    debug('dataBase class initialized:', this)
  }

  /**
   * @async
   * @function dataBase#close
   * @description Closes the connection to the database.
   */

  async close() {
    await this.connection.close()
    debug('connection closed:', this.connection)
    return
  }

  /**
   * @async
   * @function dataBase#reopen
   * @description Reopens the connection to the database if it has been closed.
   */

  async reopen() {
    this.connection = adodb.open(this.connectString, this.X64)
    debug('connection reopened:', this.connection)
    return
  }

  /**
   * @async
   * @function dataBase#runSQL
   * @param {string} SQL - The SQL that will be run.
   * @description Runs any valid SQL on the database.
   * @returns {object[]|object} If the SQL is a query, this is the result.
   */

  async runSQL(sql) {
    debug('starting SQL execution:', sql)
    debug('SQL code execution type:', sql.match('SELECT') ? 'query' : 'execute')

    const data = await this.connection[sql.match('SELECT') ? 'query' : 'execute'](sql)
    debug('SQL ran with result:', data)

    return data
  }

  /**
   * @async
   * @function dataBase#query
   * @param {string} table - The table that you are querying.
   * @param {string[]|string} [columns] - The column(s) you want to query - If left empty or as '*' it will query all columns.
   * @param {string[]|string} [rows] - The ID of the row(s) you want to query - If left empty or as '*' it will query all rows.
   * @param {string[]|string} [options] - Any additional paramiters in the query - If left empty there will be no additional paramiters.
   * @param {Boolean|object} [isUpdate=false] - Whether or not to update the selected fields, if so it is an object containing info about what columns change to what.
   * @description Runs a query based on the four paramiters described below. Here are all of it's child functions.
   * @returns {object[]} The result of the query.
   */

  async query(table, columns = '*' || [], rows = '*' || [], options = '*' || []) {
    debug('starting query with arguments:', 'table:', table, 'columns:', columns, 'rows:', rows, 'options:', options)

    function makeArray(str) {
      return typeof str === 'string' && str !== '*' ? [str] : str
    }

    columns = makeArray(columns)
    rows = makeArray(rows)
    options = makeArray(options)

    function processData(table, columns, rows, options) {
      debug('Starting data processing')

      function processColumns(columns) {
        let retval = ''
        for(const i in columns) {
          if(i != columns.length - 1) {
            retval += `${columns[i]},`
          } else {
            retval += `${columns[i]}`
            return retval
          }
        }

      }

      function processRows(rows) {
        let retval = ''
        for(const i in rows) {
          if(i != rows.length - 1) {
            retval += `ID=${rows[i]} OR `
          } else {
            retval += `ID=${rows[i]}`
          }
        }
        return retval
      }

      function processOptions(options) {
        let retval = ''
        for(const i in rows) {
            retval += ` AND ${options[i]}`
        }
        return retval
      }

      const SQLcolumns = processColumns(columns)
      const SQLrows = processRows(rows)
      const SQLoptions = processOptions(options)

      debug('Finished data processing')

      debug('Running query:', `SELECT ${SQLcolumns} FROM [${table}] ${rows === '*' && options === '*'? '' : 'WHERE'} ${rows === '*' ? '' : SQLrows}${options === '*' ? '' : SQLoptions};`)
      return `SELECT ${SQLcolumns} FROM [${table}] ${rows === '*' && options === '*'? '' : 'WHERE'} ${rows === '*' ? '' : SQLrows}${options === '*' ? '' : SQLoptions};`

    }

    const processed = processData(table, columns, rows, options)
    const data = await this.runSQL(processed)
    debug('Query ran with result:', data)

    return data
  }

  /**
   * @async
   * @function dataBase#createTable
   * @param {string} name - The name of the table that will be made.
   * @param {object} columns - The columns in the table, for each property the key is the column name and the value is the column type.
   * @param {object} [rows] - The rows to initially add to the dataBase, if left blank there will be no inital rows. - In each property the value will be the value inserted into the column, the column is determined by the order of the properties.
   * @description Creates a table based on the peramiters below.
   */


  async createTable(name, columns, rows = null) {
    debug('starting table creation with paramiters:', 'name:', name, 'columns:', columns, 'rows:', rows)

    debug('Starting data processing')
    function processColumns(columns) {
      let retval = ''
      for(const i of Object.keys(columns)) {
        i !== Object.keys(columns)[Object.keys(columns).length - 1] ? retval += `${i} ${columns[i]},\n` : retval += `${i} ${columns[i]}`
      }
      return retval
    }

    debug('Finished data processing')

    const SQLcolumns = processColumns(columns)

    debug('Creating table')

    const data = await this.runSQL(`CREATE TABLE ${name} (\n${SQLcolumns}\n);`)
    debug('Table created with result:', data)

    if(rows !== null) {
      debug('Adding records:', rows)
      await this.addRecords(name, rows)
      debug('Records added')
    }

    return data
  }

  /**
   * @async
   * @function dataBase#addRecords
   * @param {string} table - The name of the the table that the rows will be inserted into.
   * @param {object} values - The rows to add to the dataBase. - In each property the value will be the value inserted into the column, the column is determined by the order of the properties.
   * @description Adds records to a database based on the peramiters below.
   */

  async addRecords(table, values) {
    debug('starting record adding with paramiters:', 'table:', table, 'values:', values)

    debug('Starting data processing')
    const data = []
    function processValues(values) {
      let retval = ''
      for(const i of Object.keys(values)) {
        i !== Object.keys(values)[Object.keys(values).length - 1] ? retval += `${values[i]}, ` : retval += values[i]
      }
      return retval
    }

    debug('Finished data processing')

    for(const i of values) {
      const SQLvalues = processValues(i)
      debug('Inserting:', SQLvalues)
      await this.runSQL(`INSERT INTO [${table}] VALUES (${SQLvalues});`).then((result) => {
        debug('Values inserted with result:', result)
        data.push(result)
      })
    }

    debug('Finished row insertion with result:', data)

    return data
  }
}

Answer 1

Review

From a first look your code is bloated with debug noise and due to poor language feature utilization.

Looking deeper I see that poor encapsulation makes using the class DataBase ambiguous and non-intuitive.

General points

• Learn to use DevTools and don't fill your code with debugging code as it is a source of bugs and as you are not enforcing the truth of the debug calls can result in misleading debug information.

• Functions return automatically you don't need empty returns at the end of functions

• Don't add code not used. this._this = this is not used, and if you did need to use it (ie no access to this) how would you get this._this ?

• Use default parameters when you can. eg X64 (why would anyone want to run 32bit when on a 64bit OS) The parameter is an annoyance and should default to true

• Avoid single use variables. eg const data = await this.connection[sql.match('SELECT') ? 'query' : 'execute'](sql); return data can be return this.connection[sql.match('SELECT') ? 'query' : 'execute'](sql)

• In functions use arrow functions for utility functions. eg function makeArray(str) { return typeof str === 'string' && str !== '*' ? [str] : str} becomes const makeArray = str => typeof str === 'string' && str !== '*' ? [str] : str;

• Don't repeat expensive operations. This is particularly important for node.js services. Node is great for IO but JS is slow and you should always keep in mind that CPU cycles cost money. Eg for(const i of Object.keys(columns)) { i !== Object.keys(columns)[Object.keys(columns).length - 1] ? becomes const keys = Object.keys(values); for(const i of keys) { i !== keys[keys.length - 1] ? without the CPU and Memory overhead needed to create the keys array 2 times for each key`

• Become familiar with the language by studying the reference material. This is an ongoing task that will need to be maintained for the length of you career. The vast majority of your code is performing Array.join (all the process... calls). Code length is a source of bugs, always keep the code length down

• Watch the naming. SQLrows and SQLoptions should be SQLRows and SQLOptions

• JavaScript uses ";" if you do not include them they are added automatically. There are some edge cases that make the human readable source difficult to determine where or if the semicolon is inserted. Add it manually so you never need to deal with the ambiguity

• Code should be understandable without comments. It is bad practice to rely on comments to understand the code. Comments are not vetted, verifiable, and can easily modified, removed, become defunct and are thus dangerous to the understanding of the code.

The interface

• Your interface does not check state when performing behaviors

E.G.

const db = new DataBase(SQLConnect);
db.close();
const result = db.runSQL(query);  // what is expected if the db is closed.

All the calls should check if the state is valid to perform any operation. Use setting to define behaviors. eg db.autoConnect = true will have the DB connect if disconnected

• Using the class syntax has forced you into a poor encapsulation pattern

E.G.

const db = new DataBase(SQLConnect);
db.connectString = SQLConnect2;  // What now??

The connectString should be set via a setter. If the connection differs then the current connection should be closed (depending on behavioral settings)

• No error checking

Every call has a possibility of error yet none of the code vets for errors, or handles any errors gracefully

Rewrite

The rewrite does not change the behavior (apart from defaults X64 to true and added open function), removes redundant and debug code, and uses a more compact style.

This is meant as an example only and may contain many typos as I am unable to test run it.

The first thing I do when reviewing code is automatically remove comments (code should be understandable without them). Poor naming means I have had to guess as to what is contained in the many arguments passed.

It is valid to argue that "I should have read the comments.", to which I can but reply "This is only a review the code below is not meant to be accurate.".

"use strict";
const adodb = require("node-adodb");
exports.dataBase = class DataBase {
    constructor(connection, X64 = true) {
        this.connectString = connection;
        this.X64 = X64;
        this.open();
    }
    async close() {
        await this.connection.close();
    }
    open() {
        this.connection = adodb.open(this.connectString, this.X64);
    }
    async reopen() {
        this.open();
    }
    async runSQL(sql) {
        return this.connection[sql.match("SELECT") ? "query" : "execute"](sql);
    }
    async query(table, columns = "*" || [], rows = "*" || [], options = "*" || []) {
        const makeArray = str => typeof str === "string" && str !== "*" ? [str] : str;
        rows = makeArray(rows);
        options = makeArray(options);
        const SQLRows = rows === "*" ? "" : "ID=" + rows.join(" OR ");
        const SQLoptions = options === "*" ? "" : " AND " + options.join(" AND ");
        return this.runSQL(`SELECT ${makeArray(columns).join(",")} FROM [${table}] ${rows === "*" && options === "*"? "" : "WHERE"} ${SQLRows}${SQLOptions};`);
    }
    async createTable(name, columns, rows = null) {
        const data = await this.runSQL(`CREATE TABLE ${name} (\n${columns.map(col => "${i} ${columns[i]}").join(",\n")}\n);`);
        if (rows !== null) {
            await this.addRecords(name, rows);
        }
        return data;
    }
    async addRecords(table, values) {
        const data = [];
        for (const i of values) {
            await this.runSQL(`INSERT INTO [${table}] VALUES (${Object.keys(i).join(", ")});`).then(result => {
                data.push(result);
            });
        }
        return data;
    }
};

Or

"use strict";
const adodb = require("node-adodb");
exports.dataBase = class DataBase {
    constructor(connection, X64 = true) {
        this.connectString = connection;
        this.X64 = X64;
        this.open();
    }
    async close() { await this.connection.close() }
    open() { this.connection = adodb.open(this.connectString, this.X64) }
    async reopen() { this.open() }
    async runSQL(sql) { return this.connection[sql.match("SELECT") ? "query" : "execute"](sql) }
    async query(table, columns = "*" || [], rows = "*" || [], options = "*" || []) {
        const makeArray = str => typeof str === "string" && str !== "*" ? [str] : str;
        rows = makeArray(rows);
        options = makeArray(options);
        const SQLRows = rows === "*" ? "" : "ID=" + rows.join(" OR ");
        const SQLoptions = options === "*" ? "" : " AND " + options.join(" AND ");
        return this.runSQL(`SELECT ${makeArray(columns).join(",")} FROM [${table}] ${rows === "*" && options === "*"? "" : "WHERE"} ${SQLRows}${SQLOptions};`);
    }
    async createTable(name, columns, rows = null) {
        const data = await this.runSQL(`CREATE TABLE ${name} (\n${columns.map(col => "${i} ${columns[i]}").join(",\n")}\n);`);
        if (rows !== null) { await this.addRecords(name, rows) }
        return data;
    }
    async addRecords(table, values) {
        const data = [];
        for (const i of values) {
            await this.runSQL(`INSERT INTO [${table}] VALUES (${Object.keys(i).join(", ")});`).then(result => {data.push(result)});
        }
        return data;
    }
};