1
\$\begingroup\$
    function get_sql_string(dbConn,query_string,col_delim,row_delim) {
    var result = dbConn.executeCachedQuery( query_string );
    var result_string = '';
    var column_count = result.getMetaData().getColumnCount();
    if ( col_delim ){
        col_delim = col_delim;
    }
    else {
        col_delim = ','; 
    }

    if ( row_delim ){
        row_delim = row_delim;
    }
    else {
        row_delim = '\n'; 
    }

    if(result.size() > 0){
        while (result.next()) {
            i = 1;
            var line = '';
            while (i <= column_count) {
                line = line + result.getString(i) + col_delim;
                i++;
            }
            result_string = result_string + line.slice(0, -1) + row_delim;
        }
        result_string = result_string.slice(0, -1);
        result.close();
        if (result_string.toString() != 'null' || result_string != null) {
            return result_string.trim();
        }
        else {
            return '';
        }
    }
    else {
        dbConn.close();
        return '';
    }
    dbConn.close();
}

Executes a SQL select query and creates a string from the results

  • @param0 {String} dbConn - connection string for database

  • @param1 {String} query_string - query to execute

  • @param2 {String} col_delim - column delimiter separates each value in a row, default value is a commma

  • @param3 {String} row_delim - row delimiter separates earch row in the resultset, default value is a newline character

  • @return {String} returns the resultset as a string with column and row delimiters

Above is a function in that converts a SQL resultset to a string. I want to learn how to make this code more succint for maintainability. Can someone help? Thanks!

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Can you elaborate about "more efficient" please? In which particular ways of efficiency? Performance, maintainability, etc.? \$\endgroup\$ – πάντα ῥεῖ Feb 28 '20 at 17:21
  • \$\begingroup\$ I want someone to take alook at this code and ask "Why did he do it that way? Here's a simple, succint way to do it." \$\endgroup\$ – dsaini Feb 28 '20 at 17:24
  • 3
    \$\begingroup\$ "Why did he do it that way?" These things should be usually explained a priori by the author (in comments, or concepts). \$\endgroup\$ – πάντα ῥεῖ Feb 28 '20 at 17:48
  • \$\begingroup\$ I am the author. Is there a succint way to re-write this code for maintainability? \$\endgroup\$ – dsaini Feb 28 '20 at 18:26
  • 3
    \$\begingroup\$ Add such concerns for review to your question please, not in comments. \$\endgroup\$ – πάντα ῥεῖ Feb 28 '20 at 18:30
1
\$\begingroup\$

You write this is a

Javascript function to convert SQL resultset to a string

however that is not true. It's a function that

  1. executes an SQL query
  2. converts its result set to a string
  3. (sometimes) closes its DB connection

Generally its bad idea to have a function to do more than one thing. Especially the closing of a DB connection that (I presume) is passed in open is very out of place and unexpected to the user of this function, more so because it only happens when the result set of the query is empty. (The last dbConn.close() is never executed).

Conversely the result set is only closed if it isn't empty. I don't know which DB library you are using, so it may be ok, but I doubt it.

Generally you should be using try ... finally blocks to make sure you are closing your resources.


Next you have pointless assignments such as col_delim = col_delim; when handling default argument values. All current JavaScript implementations support default parameters:

 function get_sql_string(dbConn, query_string, col_delim = ",", row_delim = "\n") {

Also look into using const/let instead of var.


Another potential bug is the use of .slice(0, -1). Besides being inefficient, it assumes that the delimiters are exactly one character long, which may not be the case. At the very least you should be using .slice(0, -col_delim.length), better would be not to add this final delimiter in the first place. This could be done by collecting the results/lines in arrays and using .join(col_delim).

String concatenation (especially in a loop) is anyway very inefficient and using join would help here, too.


Other than that: Code formatting is very inconsistent. For example:

  • if( vs if (
  • if (...){ if (...) {

etc. Choose one style and stay with it (preferably the latter version). Consider using an editor/IDE that can format the code for you.


Example reimplentation (untested):

function executeQueryAndFormatResult(dbConn, queryString, colDelim = ",", rowDelim = "\n") {
  try {
    const result = dbConn.executeCachedQuery(queryString);
    return formatResult(result, colDelim, rowDelim);
  } finally {
    if (result) {
       result.close();
    }
  }
}

function formatSqlResult(result, colDelim = ",", rowDelim = "\n") {
   const columnCount = result.getMetaData().getColumnCount();
   const lines = [];
   while (result.next()) {
      const row = [];
      row.length = columnCount; // Allows JS engine to reserve the needed number of entries ahead of time
      for (let i = 1; i <= columnCount; i++) {
         row[i-1] = result.getString(i);
      }
      lines.push(row.join(colDelim));
   }
   return lines.join(rowDelim);
}

EDIT: Do the columns actually start with index 1? It seems unusual, but I've adjusted my code.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you! When I was testing my code, if I started the index at 0 it returned column names in the resultset. \$\endgroup\$ – dsaini Mar 2 '20 at 14:39
  • \$\begingroup\$ @dsaini Yes, that makes sense. I didn't think of that. \$\endgroup\$ – RoToRa Mar 2 '20 at 15:18

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.