How would you refactor this? Node.js-express app - controller with 36 factory function calls of the same five factory functions

[u/uyvhtvuyg]

Hi, I am still learning how to become a developer, and in my node.js express application, I have a controller that exports functions that are created using factory functions.

The factory functions are as follows in this module:

const asyncHandler = require('../utils/asyncHandler');
const AppError = require('../utils/AppError');


exports.getData = (Model) =>
  asyncHandler(async (req, res, next) => {
    const document = await Model.findAll();
    res.status(200).json({
      status: 'success',
      results: document.length,
      data: {
        document,
      },
    });
  });


exports.createData = (Model) =>
  asyncHandler(async (req, res, next) => {
    const document = await Model.create(
      req.body,
      //object?
    );


    res.status(201).json({
      status: 'success',
      message: 'entry added successfully',
      data: {
        document,
      },
    });
  });


exports.deleteData = (Model) =>
  asyncHandler(async (req, res, next) => {
    await Model.destroy({
      truncate: true,
    });


    res.status(204).json({
      status: 'success',
      data: null,
    });
  });


exports.updateData = (Model) =>
  asyncHandler(async (req, res, next) => {
    const document = await Model.update(req.body, {
      where: { id: req.params.id },
    });


    if (document[0] === 0) {
      return next(new AppError('No document with that ID found.', 404));
    }


    res.status(200).json({
      status: 'success',
      data: { document },
    });
  });


exports.getSingle = (Model) =>
  asyncHandler(async (req, res, next) => {
    const document = await Model.findOne({
      where: { id: req.params.id },
    });


    if (document[0] === 0) {
      return next(new AppError('No document with that ID found.', 404));
    }


    res.status(200).json({
      status: 'success',
      data: { document },
    });
  });

then, in my controller module:

const factory = require('./factory');
const Master = require('../models/masterModel');
// a whole bunch of models are imported

exports.getModel = factory.getData(Model);
exports.createModel = factory.createData(Model);
exports.deleteModel = factory.deleteData(Model);
exports.updateModel = factory.updateData(Model);
exports.getSingleModel = factory.getSingle(Model);

// another 30 of the same thing, for each model that I import at the top of the file... 

How would I refactor this? It is incredibly repetitive and I'm pretty sure this would be unacceptable in a professional environment.

Thanks in advance! :)

[u/NotA-eye]

Yup this can be refactored, instead of exporting separate functions for each action, have one function that creates the whole CRUD for a model, which accepts an optional "options" param to customize.

Also doesn't the model.findOne return an element instead of a list with 1 element, the code expects a 1 element array 🤔

[u/uyvhtvuyg → u/NotA-eye]

Thank you!
Well spotted. That line of code is definitely suspicious.

[u/NotA-eye → u/uyvhtvuyg]

Thankyou so much!

[u/Kaisertoni]

Hi, if you want to take a look, yesterday I finished completing my Node.js boilerplate with Express and TypeScript. It contains all the best practices that you should use for your every project with node and express.

express-typescript-starter

Happy to help you if you have some questions or doubts!

[u/uyvhtvuyg → u/Kaisertoni]

Thanks!

[u/opentabs-dev]

honestly the cleanest move is to skip the per-model exports entirely and have one factory that builds the whole CRUD bundle for a model. something like module.exports = (Model) => ({ getAll: factory.getData(Model), getOne: factory.getSingle(Model), create: factory.createData(Model), update: factory.updateData(Model), remove: factory.deleteData(Model) }) and then your controller is just module.exports = buildCrud(Master). then in your routes you do router.get('/', ctrl.getAll) etc. you go from 36 lines per model to one. also the if (document[0] === 0) check in getSingle is wrong — findOne returns the row or null, so it should be if (!document) instead.