3
\$\begingroup\$

I recently put together an Express/Node API that consumes and transforms an additional third party API.

And while everything is working great, I feel there may be ways to improve the architecture and implementation.

This is my current implementation:

    nodes_modules/
    routes/
    ...api.js
    ...search.js
   index.js
   .babelrc
   .config.js
   .package.json

api.js

import express from "express";
let router = express.Router();

router.use('/search', require('./search'));

module.exports = router;

search.js

import express from "express";
import axios from "axios";
import transformer from "transformer";
const router = express.Router();

router.get('/:type', (req, res) => {

    const type = req.params.type;

    axios.get('http://example.com/api/' + type)
        .then((response) => {

            let transformed = {};

            switch(type) {
                case "people":
                    transformed = transformer.search(response.data, "results[*].{ name: name }");
                    break;
                case "aircrafts":
                    transformed = transformer.search(response.data, "results[*].{ name : aircraftName }");
                    break;
                default:
                    res.status(400).send({ error : "Bad request" });
                    break;
            }

            res.send({original: response.data, transformed: transformed });
        })
        .catch( () => {
            res.status(400).send({ error : "Bad request" });
        });
});

module.exports = router;

index.js

import express from 'express';
import bodyParser from 'body-parser';
import cors from 'cors';

const app = express();
app.use(cors());
app.use(bodyParser.urlencoded({  extended: true }));
app.use(bodyParser.json());

app.use('/api', require('./routes/api'));

app.use('*', (req,res) => {
   res.status(404).send('Page Not Found');
});

app.listen(3030, function() {
   console.log('running on port 3030');
});

I appreciate any suggestions for improvement, pointing out of any flaws or useful changes that can be made to the current implementation. Thank you

\$\endgroup\$
1
\$\begingroup\$

After reading over the code snippets you included, it looks pretty simple.

The only concern I have is that if the switch statement reaches the default case, then res.status(400).send() is called , and then just after the switch statement, res.send() is called... If a 400 status is sent, then it doesn't seem appropriate to send the response data (even though transformed wouldn't be assigned in the default case.

One other improvement I see is that the 400 Bad Request response could be made consistent by moving that into a function and calling it in the two places where a 400 response is sent. So in the example below, that is defined as the function badRequest() inside the callback to router.get(), or that could be moved outside the callback and would then likely need to accept the response object as a parameter.

router.get('/:type', (req, res) => {
    function badRequest() {
        res.status(400).send({ error : "Bad API request" });
    }
    const type = req.params.type;

    axios.get('http://example.com/api/' + type)
        .then((response) => {

            let transformed = {};

            switch(type) {
                //..omitting lines for brevity
                default:
                    badRequest();
                    return; //stop the function, since 400 response sent
            }

            res.send({original: response.data, transformed: transformed });
        })
        .catch( () => {
            badRequest();
        });
});
\$\endgroup\$
  • \$\begingroup\$ But, your change to the default: handler still doesn't prevent it trying to do res.send() after you've already done badRequest(). So, it doesn't look like you fixed that part yet. You could just return after calling badRequest(), or instead of calling badRequest() in the default: handler, you could just throw and let the .catch() call badRequest(). \$\endgroup\$ – jfriend00 Aug 2 '17 at 16:08
  • \$\begingroup\$ I was mostly just trying to illustrate the abstraction of the bad request response, without fixing the former issue, but I have updated the snippet to return in the default: case. One could also argue that the default: case could be removed and if transformed is truth (i.e. <> {}) then send the JSON object, otherwise call badRequest() (or just send that 400 response there) \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 2 '17 at 16:17

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.