1
\$\begingroup\$

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

\$\endgroup\$
5
  • \$\begingroup\$ They probably mean that the service should return data (or throw exception if there were any errors) and the controller should decide what status to return and how to present that data or error to the client. \$\endgroup\$
    – slepic
    Jul 13 at 11:16
  • \$\begingroup\$ @slepic Thank you for your answer. I hear you when you are mentioning about the return data format (which I did in a middleware, but due to lack of time, I have implemented it in every services method). But Can you explained to me what does it mean that the controller is coupling (too much). I have a strong feeling that I have decoupled my code and in the service layer, I respect his purpose which is write the business logic. \$\endgroup\$
    – Eloic08
    Jul 13 at 13:14
  • \$\begingroup\$ Not to mention that I didn't call the Product Model in the Service Layer (Only Repositories). Thank you for highlighting me on this, as I am a bit confused with these explanations. \$\endgroup\$
    – Eloic08
    Jul 13 at 13:20
  • \$\begingroup\$ Welcome to the Code Review Community, the title of the question should be about what the code does, rather than your concerns about the code. \$\endgroup\$
    – pacmaninbw
    Jul 13 at 13:52
  • \$\begingroup\$ Hi @pacmaninbw, sorry for the title, should I need to rename it as "separation of concern Controller | Service | Repositories Layer"? \$\endgroup\$
    – Eloic08
    Jul 16 at 3:22
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your answer Magnus. I understand the point that the controller is pointless. Could you please guide me or share with me a repo that I can compare and see what I can improve in my code. Thank you \$\endgroup\$
    – Eloic08
    Jul 16 at 3:23
  • \$\begingroup\$ I added some example code in the edit. I don't have an open source repo that shows this easily.. \$\endgroup\$ Jul 16 at 7:13

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.