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"
});
}
