Is My Controller too Coupling?

Question

To put it in context, I had the assignment to do for a job interview in Node.JS, which basically making CRUD with products (and it have certain conditions). The assignment was well-understandable, and I have succeeded to make this task. After the company reviewed my code, I've got rejected because one of the main reasons (along with unit test not done, code not properly commented etc... ) was

• "Huge coupling of the controller" and Model is in ProductService.

I am actually confused with this sentence because I have used the Services and Repository Pattern, and separate all of the concerns, as follows (Folder structure):

• Controllers Responsible for controlling the flow of the application execution
• Models Database ORM object
• Repositories handling table data via models
• Services responsible for business logic

Product Router (import to my index router)

const router = require('koa-router')();
const ProductController = require('../../app/controllers/product.controller');
const AuthMiddleware = require('../../middleware/authValidation')

// router.get('location', ErpApiController.getLocation)

router.get('/', ProductController.searchAllProducts);                                             // GET /api/v1/products/
router.get('code/:code', ProductController.searchProductByCode);                                  // GET /api/v1/products/code/:code
router.get('name/:name', ProductController.searchProductByName);                                  // GET /api/v1/products/name/:name
router.get('brand/:brand', ProductController.searchProductByBrand);                               // GET /api/v1/products/brand/:brand
router.get('category/:category', ProductController.searchProductByCategory);                      // GET /api/v1/products/brand/:brand

Here is my Product Controller

const productService = require("../services/product.service");

exports.searchAllProducts = async (ctx) => {
  const result = await productService.getAllProducts();
  ctx.body = result;
}

exports.searchProductByCode = async (ctx) => {
  const result = await productService.getAllProductByCode(ctx.params.code)
  ctx.body = result;
}

exports.searchProductByName = async (ctx) => {
  const result = await productService.getAllProductByName(ctx.params.name)
  ctx.body = result;
}
....

Product Repository

const models = require("../models");
const Sequelize = require("sequelize");

exports.fetchAllProduct = async () => {
  return await models.Products.findAll({
    include: [
      // {
      //   model: models.Brands,
      //   as: "brand",
      //   attributes: { exclude: ['id', 'description', 'category_ID', 'createdAt', 'updatedAt'] },
      // },
      {
        model: models.ProductSizes,
        as: "sizes",
        attributes: { exclude: ['id', 'description', 'product_code', 'createdAt', 'updatedAt'] },
      },
      {
        model: models.Categories,
        as: "category",
        attributes:  ['name'],
        through: {
          attributes: [],
        }
      },
    ],
  });
}

exports.fetchProductByCode = async (code) => {
  return models.Products.findOne({
    where: {
      code: code,
    },
    include: [
      {
        model: models.ProductSizes,
        as: "sizes",
        attributes: { exclude: ['description', 'product_code', 'createdAt', 'updatedAt'] },
      },
      {
        model: models.Categories,
        as: "category",
        attributes:  ['name'],
        through: {
          attributes: [],
        }
      },
    ],
  })
}

Product Service file

const dateFormat = require("../../utils/dateUtil");
const productRepo = require("../repositories/product.repository");
const productSizeRepo = require("../repositories/productSize.repository");
const productCateRepo = require("../repositories/productCategory.repository");
const categoryRepo = require("../repositories/category.repository");
const _ = require("lodash");



exports.getAllProducts = async () => {
  return await productRepo.fetchAllProduct();
}

exports.getAllProductByCode = async (ctxcode) => {
  const data = await productRepo.fetchProductByCode(ctxcode)

  if(!data) {
    return {
      code: "204",
      success: true,
      data: [],
    };
  }

  const products = [];
    
    products.push({
      code: data.code,
      name: data.name,
      description: data.description,
      img_url: data.img_url,
      product_url: data.product_url,
      brand_name: data.brand_name,
      color: data.color,
      start_period: dateFormat.dateFormat(data.start_period),
      end_period: dateFormat.dateFormat(data.end_period),
      isActive: data.isActive,
      sizes: data.sizes,
      categories: data.category
    })

  return {
    code: "200",
    success: true,
    count: products.length,
    message: "",
    data: products,
  };
}

exports.getAllProductByName = async (name) => {
  const data = await productRepo.fetchProductByName(name)

  if(!data) {
    return {
      code: "204",
      success: true,
      data: [],
    };
  }

  const products = [];
  for(let i =0; i < data.length; i++) 
  {
    products.push({
      code: data[i].code,
      name: data[i].name,
      description: data[i].description,
      img_url: data[i].img_url,
      product_url: data[i].product_url,
      brand_name: data[i].brand_name,
      color: data[i].color,
      start_period: dateFormat.dateFormat(data[i].start_period),
      end_period: dateFormat.dateFormat(data[i].end_period),
      isActive: data[i].isActive,
      sizes: data[i].sizes,
      categories: data[i].category
    })
  }

  return {
    code: "200",
    success: true,
    data: products,
  };

}

I would like to know your thought on it and how can I improve the architecture

Answer 1

As it stands your controller is pretty much pointless since it's the service that decides on the api's output model and status codes. The service is also responsible for the db model transformation, i.e. it converts the db model directly to the api model. However, you have stated that the service is responsible for business logic. It is not setup to do this well (why? because it has to handle three models at the same time)

What I would suggest is to separate the boundaries in your system more clearly. In your case you have the api, the db, and everything in between. You could transform DatabaseProduct into Product in the repository, then process Product inside the service (your business logic), and finally transform Product into ApiProduct in the controller (with some helper functions or classes depending on your needs). Note that for a simple api these models might be the same, but it can still be helpful to think about in this way.

Finally, I would also consider extracting the transform functions into pure functions. You might find that you can eliminate some classes/modules entirely. You might also find that the list of products and single product are the same model

Edit:

I'm not familiar with koa router, and don't use sequelize, but something like below is what I'm talking about:

// repo
const rowToProduct = (row) => {
    return {
        code: row.code,
        name: row.name,
        description: row.description,
        img_url: row.img_url,
        product_url: row.product_url,
        brand_name: row.brand_name,
        color: row.color,
        start_period: dateFormat.dateFormat(row.start_period),
        end_period: dateFormat.dateFormat(row.end_period),
        isActive: row.isActive,
        sizes: row.sizes, // or a separate transform for size
        categories: row.category // or a separate transform for category
    }
}

exports.fetchProductByCode = async (code) => {
    const row = await models.Products.findOne(/* query */)
    if (!row) {
        return null
    }
    return rowToProduct(row)
}

// controller
exports.searchProductByCode = async (ctx) => {
    const product = await productRepo.fetchProductByCode(ctx.params.code)

    if (!product) {
        ctx.status = 404
        return
    }

    // like below, or a separate transform for the api response
    ctx.body = {
        code: "200",
        success: true,
        count: 1,
        message: "",
        data: [product],
    }
}

So just remove the service since you don't have any business logic (add it only if you need it). Do the transform right in the repo. Move the api model to the controller.