I've created a Node.js package that retrieves data from Icinga (a monitoring platform), formats it and passes it off to a class that generates some HTML and then sends it all out as an email.
The email, in its simplest form, looks something like this:
I come from a Ruby/Python background and this is the first time I've ever delved into Node. The code below works and does what I need it to but I have the feeling it isn't using some of Node's best practices. I make 2 API calls, after the first one happens, inside the callback, I make another API call and then when all the data is returned I invoke a function to send off the email. I'm pretty sure this could be using async/await and/or Promises but I'm just not sure where to start in order to refactor it.
// Get the data
const warning = 1;
const error = 2;
const icingaServer = new icingaApi(icingaConfig.url, icingaConfig.port, icingaConfig.username, icingaConfig.password);
const clients = [
{ 'Client 1': '**.**.client_1.**.**' },
{ 'Client 2': '**.**.client_2.**.**' }
];
const data = [];
function sendEmail() {
nodemailerMailgun.sendMail({
from: '[email protected]',
to: appConfig.sendees,
subject: 'Some subject',
html: new tableHtmlGenerator(data).run()
}).then((_res) => {
let emailAddresses = appConfig.sendees.join(', ');
console.log(`Email sent successfully to the following addresses: ${emailAddresses}`);
}).catch((err) => {
console.log(`Error: ${err.message}`);
});
}
function allDataRetrieved() {
return data.length === clients.length;
}
clients.forEach((clientMap) => {
Object.entries(clientMap).forEach(([client, hostnameWildcard]) => {
let totalHosts;
let totalServices;
let errors;
let warnings;
icingaServer.getServiceFiltered({
"filter": "match(service_name, service.host_name)",
"filter_vars": {
"service_name": hostnameWildcard
}
}, (err, res) => {
if (err) return `Error: ${err}`;
warnings = res.filter(o => o.attrs.state === warning).length;
errors = res.filter(o => o.attrs.state === error).length;
totalServices = res.length;
icingaServer.getHostFiltered({
"filter": "match(host_name, host.name)",
"filter_vars": {
"host_name": hostnameWildcard
}
}, (err, res) => {
if (err) return `Error: ${err}`;
warnings += res.filter(o => o.attrs.state === warning).length;
errors += res.filter(o => o.attrs.state === error).length;
totalHosts = res.length;
data.push({
name: `${client} (${totalHosts}/${totalServices})`,
errors: errors,
warnings: warnings
});
if (allDataRetrieved()) sendEmail();
});
});
});
});
I've omitted all the require
and const
definitions at the top of this file as they're not really needed in order to understand the code in my opinion.
The main issue is that one API call happens inside the callback of another API call and this feels nasty to me. I also wait for all the data to be pushed to the data
variable by doing a simple but crude if
statement to check if all the data has been retrieved and pushed to the array, if it has then the email is sent.
I also feel like I need to add that I'm aware this code could be improved by dumping all this business logic into a class or splitting it out into separate files. I'm not after help in that sense, it's more of how to handle API requests and waiting for the requests to finish and when/how/if to use Promises.