1
\$\begingroup\$

I have created common function for CRUD operations.


  1. I am using Sequelize JS, I have created all models in Sequelize
  2. I have written stored procedures in mysql for CRUD operations for each module/tables I have
  • GetItems
  • GetItemById
  • CreateItem
  • UpdateItem
  • Deleteitem
  1. Then I have created Common CRUD function in Utilities which has below functions.
  • Get All Data

exports.getAllData = (spname, res, next) => {
    sequelize
        .query(
            spname,
            Sequelize.QueryTypes.SELECT)
        .then(result => {
            res.status(200).json({
                message: "data Fetched from database",
                statusCode: 200,
                result: result,
            });
        })
        .catch(error => {
            if (!error.statusCode) {
                error.statusCode = 500;
            }
            next(error);
        });
}

  • Get Data by ID

exports.getDataById = (spname, parameter, res, next) => {
    sequelize.query(
        spname,
        {
            replacements: parameter
        },
        Sequelize.QueryTypes.SELECT
    )
        .then(result => {
            res.status(200).json({
                message: "data Fetched from database",
                statusCode: 200,
                result: result,
            });
        })
        .catch(error => {
            if (!error.statusCode) {
                error.statusCode = 500;
            }
            next(error);
        });
}

  • Create Item

exports.postData = (spname, parameters, res, next) => {
    sequelize.query(
        spname,
        {
            replacements: parameters
        },
        Sequelize.QueryTypes.INSERT
    )
        .then(result => {
            res.status(200).json({
                message: "data added to database",
                statusCode: 200,
                result: result,
            });
        })
        .catch(error => {
            if (!error.statusCode) {
                error.statusCode = 500;
            }
            next(error);
        });
}

  1. Call that functions in Controller functions

const Category = require("../models/category");

const crud = require('../utility/crud');

exports.getAllCategories = (req, res, next) => {
  crud.getAllData('call USP_GetAllCategories()', res, next);
}

exports.getCategoryById = (req, res, next) => {
  let parameter = {
    categoryId: req.params.id
  }
  crud.getDataById('CALL USP_GetCategoryById(:categoryId)', parameter, res, next);
}

exports.addCategory = (req, res, next) => {
  let parameter = {
    name: req.body.name,
    image: req.body.image,
    createdBy: req.body.createdBy
  }
  let query = 'CALL USP_CreateCategory(:name,:image,:createdBy)';
  crud.postData(query, parameter, res, next);
}


I have below questions about it.

  1. Is it good approach to create stored procedures and call them in application with / without using Sequelize?
  2. Is it good practice to create common function for CRUD? or should go with writing function for each module ?(I checked on internet, I never found any good satisfactory solution about this)
  3. Could you please help to review code so I can make it more better ?
\$\endgroup\$
1
1
\$\begingroup\$

Your first two questions:

  • You don't appear to be using almost a single feature of sequelize, so I can't understand why you would use it.
  • What you have done is to put all your logic in the database, and have an application that's just a proxy for the database. If your application needs to evolve even a little bit, this can quickly fall apart. Stored procedures can be a pain to refactor. Often you don't know where and how they are used. Also, oftentimes there is no version control. So what happens is that they grow bigger, or workarounds are added elsewhere, further cementing the spaghetti. Even worse, there are many things you just cannot do with stored procedures. It won't take long before you will add some logic to the application, and then you have it split between the application and database, making it even harder to work with.
  • "Is it good practice to create common function for CRUD" - If you KNOW that EVERY single case, now and in the future, will be the EXACT same, then yes. Otherwise, it's a much better idea to create good abstractions from the get-go, and accept some boilerplate. It often turns out that cases that look the same are in fact not the same, and then it's much easier to adjust the already separate cases than to add conditionals to the crud god class.

Review:

  • The crud module exposes spname as a parameter. It appears this is the query to call a stored procedure. It's a pretty leaky abstraction.. why does the controller need to know anything about the stored procedure? Personally I would not use stored procedures like this, but if I had to I would at least limit the exposure to a named resource and its parameters.
  • The crud module mixes controller and database logic. Evolving such an abstraction is hell. A few years of features and you'll have a pile of conditionals to take care of all the cases you haven't thought about yet.
  • All error status codes are set to 500. What about NOT FOUND errors?
  • Where's the validation?
\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your comments - For not found, I have added at stored procedure level - I am checking id passed to parameter, that record is exists or not.. if not exists then I am showing message as "Record with mentioned Id does not exists" and for validation, I am handing validations in routers using "express-validator" for it. \$\endgroup\$ Aug 1 at 2:13
  • \$\begingroup\$ how should I call store procedure from controller? \$\endgroup\$ Aug 1 at 2:19

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.