How could you prevent controller callback methods from becoming too big?

[HappyZombies]

trafficstars

Hello! I've been following this pattern for my personal project, but I am reaching a point where I think that the controller layer (the express routes) is getting a bit too big, below is an example from one of your articles.

export default (app) => {
  app.get('/user/search-location', (req, res, next) => {
    try {
      const { lat, lng } = req.query;
      Logger.silly('Invoking user service to search by location')
      const users = UserService.SearchUserByLocation(lat, lng);
      return res.json(users).status(200);
    } catch(e) {
      Logger.warn('We fail!')
      return next(e);
    }
  })
}

Say, I need this endpoint to do A LOT and it's fairly complicated. It needs too query different tables, databases and each service that this calls relies on a previous service. Not to mention all the logging I need to do.

Would make it sense to separate the callback method to maybe a controller directory, and if so how would that look like?

Should I have another service that handles all the complicated parts.

Or should I separate it all into middlewares?

Basically, if my controller method callback is looking big, what steps should I take to reduce or separate it?

[santiq]

Hi @HappyZombies

The controller should only have 1 call to a service that handles all the business logic required for that endpoint.

I know that a search endpoint is one of the most complicated ones because you pass several filters.

If you need to call several services, you that may run into circular dependency injections. To avoid that, you could try to apply the Facade Pattern (read this related issue https://github.com/santiq/bulletproof-nodejs/issues/7)

Essentially is just a service that aggregates several services.

Let's see an example, suppose you have a service that gets Venues/Places from third-party providers (google places, facebook places graph, yelp, etc) and your own database. Then, using the twitter api you get all twits near a place.

import PlaceService from './PlaceService'
import GooglePlaceService from './GoogleAPIService'
import FBPlacesGraphService from './FBPlacesService'
import YelpService from './YelpService'
import TwitterService from './TwitterService'

class SearchService {
 constructor(
   private placeService: PlaceService,
   private googlePlaceService: GooglePlaceService,
   private fbPlacesGraphService: FBPlacesGraphService,
   private yelpService: YelpService,
   private twitterService: TwitterService
 ) {}
 
  async  public Search(filters, location) {
    const allPlaces = await Promise.all([this.searchInFacebook(filters, location), this.searchInGoogle(filters, location), this.searchInYelp(filters, location), this.searchInOwnDB])     

    const filteredPlaces = await allPlaces.map(this.removeDuplicates)

    const twitts = await this.getTwitsNearALocation(location)
    
    const placesWithTwits = await this.mergeAllTogether(filteredPlaces, twits);
    return result;
   }

   private getTwitsNearALocation(location) {

   }
   private searchInFacebook(filters, location){ 

   }

   private searchInGoogle(filters, location){ 

   }

   private searchInYelp(filters, location){

   }
   private searchInOwnDB(filters, location){

   }

   private mergeAllTogether(places, twits) {

   }

   private removeDuplicates(places) {

   }
}

What do you think?

[HappyZombies]

Ah I get it! Makes complete sense then, I just had this thinking that a service was only meant to retrieve/do things with database layer stuff. But the service layer is meant to do any logic for that particular endpoint and, like you said, if the service gets to big, then follow the facade pattern. Makes sense to me!

It seems I was really just under thinking how the services need to be used, I should use them to their full extant.

Thanks!

[varunagarwal315]

I tried splitting code like this but I get an error saying this is undefined inside the class. Any idea what may cause this? Tried logging this in the constructor function and still showed as undefined. Even tried using .bind() in the router while calling the controller but didn't work.

[HappyZombies]

Hey I can help. This has happened to me before. If you’re using regular Node (no typescript) make sure your routes callback are using arrow functions. Could you give an example of what your code looks like?

[varunagarwal315]

I used .bind() and it is now working. I think when passing the controller as a middleware the context for this inside the controller class comes as undefined. My current setup is index.js -- Using koa-router

const FundController = require('./controllers');
const CreateFundFacade = require('./services/create-fund');
const SearchFundFacade = require('./services/search-fund');
const FundService = require('./services');

// Injecting the stuff here. DB models are directly imported, not using injections now
const createFundFacade = new CreateFundFacade();
const searchFundFacade = new SearchFundFacade();
const fundService = new FundService(createFundFacade, searchFundFacade);
const fundController = new FundController(fundService);

router.post('/', validateSession, fundController.createFund.bind(fundController));

My controller

class FundController {
  constructor(FundService){
      this.fundService = FundService;
   }

  async createFund(ctx) {
    try {
      const result = await this.fundService.createFund(ctx.request.body);
      return ctx.send(200, {
        message: 'Fund has been added successfuly',
        fundID: result.fundID
      });

    } catch (err) {
      console.log('Error at create fund');
      console.log(err);
      return ctx.send(500, 'Something went wrong');
    }
  };

  module.exports = FundController;
}

The createFundFacade

class CreateFundFacade {

  createFund(data) {
    return new Promise(async (resolve, reject) => {
      try {
        const {
          // lot of data
        } = data;
        const fund = new FundModel({
          // create mongoDB object from data
        });
        resolve(await fund.save());

      } catch (err) {
        reject(err);
      }
    });
  };
};
module.exports = CreateFundFacade;

And finally the service

class FundService {

  constructor(CreateFundFacade, SearchFundFacade){
    this.createFundFacade = CreateFundFacade;
    this.searchFundFacade = SearchFundFacade;
  }

  createFund(body) {
    return this.createFundFacade.createFund(body);
  };

  getFundByID(body) {
    return this.searchFundFacade.getFundByID(body);
  };

  getFundsByInvestorID(investorID) {
    return this.searchFundFacade.getFundsByInvestorID(investorID);
  };
};

module.exports = FundService;

Not sure if I am doing this right. Couldn't find any solid examples for creating the facade pattern

[HappyZombies]

Yup, using bind will help with the ctx from koa. Also, personal opinion, but I don't seperate out the controllers since I don't want to pass the ctx to different classes, I want to keep it all contained. You can see how I did it in my personal project for context . Again personal opinion! In order to handle different errors I have a "ApiError" class that extends Error, and simply call next() whenever I throw it

[varunagarwal315]

Understood, but when you need to pass multiple middleware such as validation for token, or permission and roles, won't you need to split the functions into individual controllers?

On Tue, Feb 4, 2020 at 5:48 PM Daniel Reguero [email protected] wrote:

Yup, using bind will help with the ctx from koa. Also, personal opinion, but I don't seperate out the controllers since I don't want to pass the ctx to different classes, I want to keep it all contained. [You can see how I did it in my personal project for context.]( https://github.com/HappyZombies/express-backend-starter/blob/master/api/routes/v1/users.js . Again personal opinion! In order to handle different errors I have a "ApiError" class that extends Error, and simply call next() whenever I throw it

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/santiq/bulletproof-nodejs/issues/31?email_source=notifications&email_token=AC5Y362VSVPYC2VPR4MN6I3RBFMIRA5CNFSM4ILYKZ52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKXNW4I#issuecomment-581884785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5Y36ZMVUH7GOVXHALYYB3RBFMIRANCNFSM4ILYKZ5Q .

[santiq]

@varunagarwal315 I would have my services instance inside the controller function, that way a new instance per request is created, it is crucial if you use any kind of state inside the class.

Here is more information about it...

https://stackoverflow.com/questions/47664079/node-js-express-and-object-instantiation-inside-outside-route-endpoints

[varunagarwal315]

@santiq Thanks for the link. Yes I currently don't have any state but once I require it, will definitely shift. What do you think about moving the ctx/ request context out of the route to multiple controllers and chain them up, similar to how the chain of responsibility concept.

Thanks once again. Learning a lot from this repo

On Tue, Feb 4, 2020 at 6:25 PM Sam Quinn [email protected] wrote:

@varunagarwal315 https://github.com/varunagarwal315 I would have my services instance inside the controller function, that way a new instance per request is created, it is crucial if you use any kind of state inside the class.

Here is more information about it...

https://stackoverflow.com/questions/47664079/node-js-express-and-object-instantiation-inside-outside-route-endpoints

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/santiq/bulletproof-nodejs/issues/31?email_source=notifications&email_token=AC5Y366PHL376BPIINZFSR3RBFQTJA5CNFSM4ILYKZ52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKXQWEQ#issuecomment-581896978, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC5Y3652K233EJODL6ZMUU3RBFQTJANCNFSM4ILYKZ5Q .

[varunagarwal315]

@HappyZombies Hey in this pattern have you tried using arrow functions inside the controller and service class instead of defining the functions. Eg

async doSomething(req, res, next) { }

becomes

doSomething = async (req, res, next) => { }

This allows us to avoid using bind() in the router file. It seems a bit cleaner but read in a few stackoverflow answers that it is advised to avoid using arrow functions in any class. Thoughts?