0
\$\begingroup\$

I'm new to javascript and trying to figure out what improvements I can make to my code.

In this code I retrieve values ​​from an iODBC database then I format and push in SQL database. I am making arrays to get all my values that I am processing, however I think the best way would be to do it in object. I don't know if this is useful or too heavy?

function refreshPrice(){
    cron.schedule('* 20 * * *', () => {
      console.log("refresh prix CLIP");
      var articlesClip = null;
      var codeGPAO = [], priceGPAO = [], pmpGPAO = [], dateGPAO = [];
      // Set de la query + exec pour récuperer les infos dans la BDD CLIP
      var command = 'echo "SELECT COARTI,PRIXART,PMP,DAT FROM ARTICLEM" | iodbctest "DSN=DSNServ"';
      var exec = require('child_process').exec;
      var child = exec(command, {maxBuffer: 10000 * 1024}, function (error, stdout, stderr) {
        var articlesClip = stdout.toString("utf8").split('\n');
        for (var i = 0; i < 8; i++){
          articlesClip.shift();
        }
        for (var i = 0; i < 7; i++){
          articlesClip.pop();
        }
        console.log(articlesClip)
        for (var articleID in articlesClip) {
          articlesClip[articleID] = stdoutConvert(articlesClip[articleID]);
          var str = articlesClip[articleID];
          var tab = pipSplit(str);
          codeGPAO.push(tab[0].trim());
          priceGPAO.push(tab[1].trim());
          pmpGPAO.push(tab[2].trim());
          dateGPAO.push(dateFormatFR(tab[3]));
        }
        for (var i = 0; i < codeGPAO.length; i++){
          var query = "UPDATE COMPOSANTS SET \
            PRIX_CLIP=" + mysql.escape(priceGPAO[i]) + ", \
            PRIX_PMP=" + mysql.escape(pmpGPAO[i]) + ", \
            DATE=" + mysql.escape(dateGPAO[i]) + ""
            query += " WHERE GPAO=" + mysql.escape(codeGPAO[i])
          console.log(query);
          connection.query(query, function (error, results, fields) {
            if (error) throw error;
          });
        }
        console.log("FINISH");
      });
    },{
      scheduled: true,
      timezone: "Europe/Paris"
    });
  }
\$\endgroup\$
3
  • 3
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Nov 6 '20 at 7:35
  • \$\begingroup\$ Please edit your question and explain the code thoroughly \$\endgroup\$
    – Parekh
    Nov 6 '20 at 19:03
  • \$\begingroup\$ Hello, no idea to edit my title, any idea ? \$\endgroup\$
    – uPong
    Nov 9 '20 at 12:35
5
\$\begingroup\$

When you have multiple standalone variables that hold strongly related data, often it's a good idea to coalesce those variables into a single variable that holds the entire collection, which permits more dynamic and DRY manipulation of the data. Here, your codeGPAO, priceGPAO and similar variables fall exactly into this category; you're right, putting each parsed line into an object instead of 4 standalone variables would make a lot more sense.

For example, you could do:

const GPAOs = [];
for (var articleID in articlesClip) {
    // ...
    GPAOs.push({
        code: tab[0].trim(),
        price: tab[1].trim(),
        pmp: tab[2].trim(),
        date: dateFormatFR(tab[3]),
    });
}

That data structure would make a lot more sense.

I don't know if this is too heavy?

Not at all. It'll have the same computational complexity and the same basic operations. Except in ultra-performance-critical code (which is exceedingly rare to run into in real-world JavaScript), the performance difference between using an object and standalone variables is completely insignificant. And since it'll make the code more comprehensible to readers, it's definitely worth it.

That said, there's an even better option available to you - why store the results at all? I think it'd make more sense to send off the database query immediately once the string has been parsed - that way, there's no need for any of the GPAO variables at all. For example:

for (var articleID in articlesClip) {
    // ...
    updateDatabase({
        code: tab[0].trim(),
        price: tab[1].trim(),
        pmp: tab[2].trim(),
        date: dateFormatFR(tab[3]),
    });
}

There are many other improvements that can be made to the code as well.

Use modern syntax Since this is running in Node, and you're using arrow functions already, there shouldn't be any compatibility problems with using modern syntax everywhere. Most importantly - use const (or let when you need to reassign), not var. var has unintuitive function-scoping rules and a few other odd behaviors, and const makes code more readable when it's clear at a glance when a variable won't be reassigned, so it's a good idea to use const wherever possible. Consider using the ESLint rules no-var and prefer-const.

Only declare variables right before you need to use them It's good for variables to exist in as narrow a scope as possible. This improves readability because it means that when reading an outer block, you don't need to read over and cognitively parse variable declarations that are only used inside an inner block. You can remove the var articlesClip = null; completely, and only declare it once the stdout has been parsed with stdout.toString("utf8").split('\n');.

Don't ignore errors With your current implementation, if the exec call doesn't succeed, you will not know what the error was - you don't even check it - and then you proceed to try to parse the stdout anyway. Instead, check to see if there's an error first - if there is one, log it (somewhere that you can find it easily when reading server logs) and return. Otherwise, proceed with parsing.

Concisely taking a slice of an array Rather than looping 8 and 7 times, .pop and .unshifting items from the array, consider using .slice instead, which allows you to grab a section of an array between two indicies. Change the for loops to:

.slice(8, articlesClip.length - 7)

If you only care about the values of an array, avoid for..in - for..in lets you iterate over the indicies of an array, but when you don't care about the indicies, only the values, better to iterate over the values themselves, which can be done with for..of or forEach. You can change

for (var articleID in articlesClip) {

to

for (const articleStr of articlesClip) {

Or - since you want to know when all queries are done, use Array#map instead, to map each articleStr to a database query Promise that you can pass into Promise.all.

Don't concatenate input when performing database queries It's not only inelegant, it also opens you up to SQL injection (either purposeful or accidental) when done wrong. Using mysql.escape is better than nothing, but it's still not a good idea. Use prepared statements instead.

How to do this depends on your database and database driver. For example, with postgresql, syntax like the following can be used:

db.query(
    `
        SELECT username
        FROM account
        WHERE userid = $(userid);
    `,
    { userid }
)

Look up the docs for your database to figure out how it can be used with prepared statements (also known as parameterized queries).

"Finish" finishes at the wrong place Your finish log is outside of the connection.query callback, so it'll be logged before any of the queries are done. If you want to wait for all asynchronous operations to finish, you should use Promise.all, which will require the database query to return a Promise instead of using a callback. (Promises are generally nicer to use than callbacks anyway - they require less nesting, and error handling is easier.)

In all, I'd hope to be able to refactor the code to something like this:

const updateDatabaseQueryString = `
    UPDATE COMPOSANTS
        SET
            PRIX_CLIP=$(price),
            PRIX_PMP=$(pmp),
            DATE=$(date)
    WHERE GPAO=$(code);
`;
const parseLine = (articleLine) => {
    const str = stdoutConvert(articleLine);
    const tab = pipSplit(str);
    // Use a promisified version of connection.query:
    return connection.query(updateDatabaseQueryString, {
        code: tab[0].trim(),
        price: tab[1].trim(),
        pmp: tab[2].trim(),
        date: dateFormatFR(tab[3]),
    });
};
const refreshPrice = () => {
    const command = 'echo "SELECT COARTI,PRIXART,PMP,DAT FROM ARTICLEM" | iodbctest "DSN=DSNServ"';
    // Import the promisified version of .exec:
    const exec = util.promisify(require('child_process').exec);
    exec(command, { maxBuffer: 10000 * 1024 })
        .then((stdout) => {
            const stdoutLines = stdout.toString("utf8").split('\n');
            const articlesClip = stdoutLines.slice(8, stdoutLines.length - 7);
            return Promise.all(articlesClip.map(parseLine));
        })
        .then(() => {
            console.log('All prices updated successfully');
        })
        .catch((error) => {
            console.log('Error updating prices:', error);
        });
};
function scheduleRefreshPrice() {
    cron.schedule('* 20 * * *', refreshPrice, {
        scheduled: true,
        timezone: "Europe/Paris"
    });
}
\$\endgroup\$
6
  • \$\begingroup\$ I say a big thank you. Thank you for spending so much time answering my question. You answered all the questions I asked myself. And your code is "perfect". So that I can learn, I will not copy your code from A to Z but I will study it and spot my mistakes. You made me a lesson and I thank you again. Just a quick question, the exec child_process won't crash with all promises? \$\endgroup\$
    – uPong
    Nov 9 '20 at 12:33
  • \$\begingroup\$ If your original code doesn't have a problem with sending out all requests at once, then this code, which is doing the same thing, shouldn't have an issue with it either. \$\endgroup\$ Nov 9 '20 at 14:07
  • \$\begingroup\$ Ok thank ! Mocha is the best way to test this code ? \$\endgroup\$
    – uPong
    Nov 9 '20 at 14:48
  • \$\begingroup\$ I barely have any experience with testing frameworks, but I don't see anything here that would interfere with that. Any of the well-used standard testing libraries should be able to test it (at least until the point of database insertion). \$\endgroup\$ Nov 9 '20 at 14:50
  • \$\begingroup\$ Nice ! because i run your code and nothing appears in my database. I have an empty array. So I would like to run a test to see where the problem is in order to correct it. @CertainPerformance \$\endgroup\$
    – uPong
    Nov 10 '20 at 8:14

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.