6

I have an express app that gets its data from an external API

api.com/companies (GET, POST)
api.com/companies/id (GET, PUT)

I want to create a model to make the code easier to maintain as you can see I´m repeating a lot of code here.

router.get('/companies', function(req, res, next) {

    http.get({
        host: 'http://api.com',
        path: '/companies'
    }, function(response) {
        var body = '';
        response.on('data', function(d) {
            body += d;
        });
    });

    res.render('companies', {data: body});
});

router.get('/companies/:id', function(req, res, next) {

    http.get({
        host: 'http://api.com',
        path: '/companies/' + req.params.id
    }, function(response) {
        var body = '';
        response.on('data', function(d) {
            body += d;
        });
    });

    res.render('company', {data: body});
});

How can I do that?

SiHa
  • 7,830
  • 13
  • 34
  • 43
handsome
  • 2,335
  • 7
  • 45
  • 73

3 Answers3

3

First of all: http.get is asynchronous. Rule of thumb: When you see a callback, you are dealing with an asynchronous function. You can not tell, if the http.get() and its callback will be completed when you terminate the request with res.render. This means that res.render always needs to happen within the callback.

I am using ES6 syntax in this example.

// request (https://github.com/request/request) is a module worthwhile installing. 
const request = require('request');
// Note the ? after id - this is a conditional parameter
router.get('/companies/:id?', (req, res, next) => {

    // Init some variables
    let url = ''; 
    let template = ''

    // Distinguish between the two types of requests we want to handle
    if(req.params.id) {
        url = 'http://api.com/companies/' + req.params.id;
        template = 'company';
     } else {
        url = 'http://api.com/companies';
        template = 'companies';
     }

    request.get(url, (err, response, body) => {
        
        // Terminate the request and pass the error on
        // it will be handled by express error hander then
        if(err) return next(err);
        // Maybe also check for response.statusCode === 200

        // Finally terminate the request
        res.render(template, {data: body})
    });

});

Regarding your 'model' question. I would rather call them 'services' as a model is some collection of data. A service is a collection of logic.

To create a company service module could do this:

// File companyService.js
const request = require('request');

// This is just one of many ways to encapsulate logic in JavaScript (e.g. classes)
// Pass in a config that contains your service base URIs
module.exports = function companyService(config) {
    return {
        getCompanies: (cb) => {
            request.get(config.endpoints.company.many, (err, response, body) => {
                return cb(err, body);
            });
        },
        getCompany: (cb) => {
            request.get(config.endpoints.company.one, (err, response, body) => {
                return cb(err, body);
            });
        },
    }
};


// Use this module like
const config = require('./path/to/config');
const companyService = require('./companyService')(config);

// In a route
companyService.getCompanies((err, body) => {
    if(err) return next(err);

    res.render(/*...*/)
});
DanielKhan
  • 1,190
  • 1
  • 9
  • 18
2

There is no need to have multiple routes in this case! You can make use of an optional parameter using ?. Take a look at the following example:

router.get('/companies/:id?', function(req, res, next) {
    var id = req.params.id;

    http.get({
        host: 'http://api.com',
        path: '/companies/' + id ? id : ""
    }, function(response) {
        var body = '';
        response.on('data', function(d) {
            body += d;
        });
    });

    res.render('companies', {data: body});
});

The bit of code here:

path: '/companies/' + id ? id : ""

Is using an inline if statement, so what it's saying is, if id != null, false, or undefined, add id to the companies/ string and else simply add nothing.

Edit

Regarding js classes, you could do something like this:

// Seperate into a different file and export it
class Companies { 
    constructor (id) {
        this.id= id;
        this.body = "";
        // You can add more values for this particular object
        // Or you can dynamically create them without declaring here 
        // e.g company.new_value = "value"
    }

    get (cb) {
        http.get({
            host: 'http://api.com',
            path: '/companies/' + this.id ? this.id : ""
        }, function(response) {
            response.on('data',(d) => {
                this.body += d;
                cb (); // callback
            });
        }); 
    }

    post () {
        // You can add more methods ... E.g  a POST method.
    }
    put (cb) {
        http.put({
            host: 'http://api.com',
            path: '/companies/' + this.id ? this.id : "",
            ... Other object values here ...
        }, function(response) {
            response.on('data',(d) => {
                ... do something here with the response ...
                cb(); //callback 
            });
        }); 
    }
}

Your router class could then use this class like so:

router.get('/companies/:id?', function(req, res, next) {
    var id = req.params.id;
    var company = new Companies(id);
    company.get(() => {
        // We now have the response from our http.get
        // So lets access it now!
        // Or run company.put(function () {...})
        console.log (company.body);
        res.render('companies', {data: company.body});
    });

});

I've added callbacks here for simplicity but I recommend using promises: https://developers.google.com/web/fundamentals/getting-started/primers/promises

James111
  • 15,378
  • 15
  • 78
  • 121
  • yup you are right, this can be simplified a lot with your approach I was looking for something like `var data = new companies()` or `var data = new companies(1)` and this companies() will do the magic to make the http request and get the data. so I can reuse it in more places. thanks!! – handsome Oct 19 '16 at 04:38
  • No worries & Good luck :) Yep, you can implement this logic into a class quite easily! I'll try add a little example. Remember +1/accept if it helped :D @handsome – James111 Oct 19 '16 at 04:42
  • @handsome Take a look at my update. It uses nodes es6 classes :) – James111 Oct 19 '16 at 04:48
  • Good idea but there's two issues with your edit :) First thing is that `company.get` happens asynchronously, so `company.body` wont have the expected value right after the call to `company.get`. Actually, it will always be `""` because you're using `this.body` in a regular function, not a method on the class, and `this` in a regular function in JS is either the global object or `undefined` in strict mode, or `response.on` could maybe use `call/apply`, changing `this` to something else. Either way, `body` that belongs to the class never changes. – nem035 Oct 19 '16 at 04:56
  • 1
    It wasn't suppose to be a copy paste job, I just whipped it up quickly! Thanks for pointing that out anyway, I've added callbacks to work around the async functionality that http gives us. @nem035 The class thingo is there more for a guide :D – James111 Oct 19 '16 at 05:02
  • Still wont work :). You need to change `function(response)` and `function(d)` to arrow functions like `(d) =>`, so `this` still points to the class and correctly updates `this.body`, or perhaps use the [self pattern](http://stackoverflow.com/questions/4371333/is-var-self-this-a-bad-pattern) – nem035 Oct 19 '16 at 05:04
1

The general way to approach refactoring is to identify what's different in your code and extract that to be a dynamic part that is passed into a function that contains the common code.

For instance, the two things that are different here are the path on which the requests happen

'/companies' vs '/companies/:id'

And the related path that gets passed to http.get

'/companies' vs '/companies/' + req.params.id

You can extract these and pass them into a function that will assign a handler for you.

Here's a generic approach:

// props contains the route and 
// a function that extracts the path from the request
function setupGet(router, props) {
  router.get('/' + props.route, function(req, res, next) {

    http.get({
      host: 'http://api.com',
      path: props.getPath(req)
    }, function(response) {
      var body = '';
      response.on('data', function(d) {
        body += d;
      });
    });

    res.render('company', {
      data: body
    });
  });
}

And then call it with the two options:

setupGet(router, { 
  route: 'companies', 
  getPath: function(req) {
    return 'companies';
  }
});

setupGet(router, { 
  route: 'companies/:id', 
  getPath: function(req) {
    return 'companies' + req.params.id;
  }
});

The benefit here is that you can use any combination of routes and paths as well as use other req properties to determine the path.

Another thing you need to realize is that your res.render call will happen before you do body += d, because the former happens synchronously right after the call to http.get and the latter happens asynchronously (sometime later).

You probably want to put the render method in the callback itself.

// props contains the route and 
// a function that extracts the path from the request
function setupGet(router, props) {
  router.get('/' + props.route, function(req, res, next) {

    http.get({
      host: 'http://api.com',
      path: props.getPath(req)
    }, function(response) {
      var body = '';
      response.on('data', function(d) {
        body += d;

        // probably want to render here
        res.render('company', {
          data: body
        });
      });
    });
  });
}
nem035
  • 34,790
  • 6
  • 87
  • 99
  • good point. I´m still getting used to this async stuff i will not be able to create one model so I can call /companies in more than one page? I´m asking because the async will need to complete the request-response in order to get data. every time I will need to display all companies I will need to make an http request and send the data in the callback? – handsome Oct 19 '16 at 04:38
  • Well, if the companies aren't changing, you can store them in memory somewhere and make the request first time, save the companies, and then every time after that just return the saved value. Do you want me to edit my question and show you an example? – nem035 Oct 19 '16 at 04:39
  • that makes sense but I´m looking for something more model oriented, like the good old PHP so I can reuse logic instead of making all those http calls everywhere. thanks man! – handsome Oct 19 '16 at 04:41
  • you're welcome amigo, [here's another possibly useful question to look at](http://stackoverflow.com/questions/19619936/how-to-separate-the-routes-and-models-from-app-js-using-nodejs-and-express) – nem035 Oct 19 '16 at 04:43