2
\$\begingroup\$

I am currently learning how to create nodejs express rest api using async making it fully scaleable and secure.

My goal is to ensure maximum functionality and speed, currently I am using MVC structure with route, controller, service and model.

Database is mysql and using mysql2 pool.

I am looking to know if the example snippets of code below are optimised as best as possible and correctly using async or is there a more efficient way ?

Route

'use strict';

const express = require('express');
const app = express.Router();
const Stats_controller = require('../controllers/stats');


app.get('/stats', Stats_controller.getStats);

module.exports = app;

Controller (unsure here about the async how its been used)

'use strict';
const statsService = require('../services/stats.js');

const getStats = async (req, res, next) => {
  statsService.getStats(function(err, result) {
    if (err)
      res.send(err);
      res.json(result);
  });
};

module.exports.getStats = getStats;

Service

'use strict';
var coinsModel = require('../models/stats.js');

const getStats =  async function(result) {
    const res = await coinsModel.getStats();
    var responseData = {
        "status":"success",
        "data":res
    }
    result(null, responseData);
}

module.exports.getStats = getStats;

Model

'use strict';
var db = require('../config/conf.js');

const getStats = async function() {
  return new Promise((resolve) => {
        db.connection.query("SELECT * FROM stats", function (err, res) {
      if(!err){
        resolve(res)
      }else{
        resolve([])
      }
    });
  });
}

module.exports.getStats = getStats;
\$\endgroup\$
1
\$\begingroup\$

Some comments:

  1. Use the built-in database promise support in mysql2 rather than promisify stuff yourself.
  2. In Model, don't hide low level database errors. If the database has an error, let the caller see the error. It's up to the caller what to do with errors.
  3. There's no reason for your Model.getStats() to be async.
  4. Service should use promises to communicate back the results, not a callback. That's the modern way to handle asynchronous results. When you already have a promise, don't introduce a plain callback.
  5. In Controller.getStats(), when you get an error and do res.send(err) and then res.json(result). You can't call both of those. You send one response and only one response for each request. Then, your code doesn't send any response when there is no an error.
  6. Log every error somewhere so you can diagnose server problems when they are occuring.
  7. As currently shown, there's no reason for Controller.getStats() to be async. You only need a function to be async if you need to use await or you're trying to use async to catch synchronous exceptions for you. It appears you are throwing async in the function definition any time you have an asynchronous operation. That's not the right reason to use async on your functions.
  8. Performance/speed of this type of code is largely going to be determined entirely by your database configuration and performance as there's nothing in this code that really influences the performance/speed much. You aren't doing any processing of the data here. The extra Service layer isn't adding much value, but also probably doesn't really change much compared to the DB performance.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ So basically - Controller = remove async. - Model = remove async and change to mysql2 promise - Service = async but instead of callback use promise \$\endgroup\$ – Jojo Bagings Jan 1 at 20:46
  • \$\begingroup\$ @JojoBagings - I'd say "use async when appropriate". If your function implementation drives a need for it (usually driven by the desired use of await in the function), add it - otherwise don't. \$\endgroup\$ – jfriend00 Jan 1 at 20:58

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.