node.js architecture suggestions

Question

I have a small node.js app (MY FIRST)

I have models using mongoose, controllers for each and a seperate route, I feel like this is pretty good seperation but something about the number of route files i would have to create seems not route, basically every section of the restful api would have a route, model and controller, this keeps everything nice and tidy but I had a few question or wishes for recommendations

The complete code can be found here https://github.com/ninjatalent/starternodejs

however i will detail out one specific area i have questions below

in my app.js i have

app.use('/api/issue', issueWebsiteRouter);
app.use('/api/project', projectRouter);

This is because i can specify a router to handle /issue or /project, but this means that I will create a router for each of this. Is this fairly common? Good architecture? Let me show you an example of a router

var express = require('express');

var route = function(Issue){

var issueWebsiteController = require('../controllers/issueWebsiteController')(Issue)  // putting elements in controllers to make it easier for unit testing etc, inject mongoose Issue model
var appRouter = express.Router();
appRouter.route('/')
    .post(issueWebsiteController.post)
    .get(issueWebsiteController.get);

// this is some middleware we are injecting when we are looking at an id we see if there is a book, we add it to request and next moves us to the next method
appRouter.use('/:id', function(req,res,next){
    Issue.findById(req.params.id, function(error,issue){
        if(error)
        {
            res.status(500).send(error);
        }
        else if(issue)
        {
            req.issue = issue;
            next();

        }
        else
        {
            res.status(404).send('no book found');
        }
    });
});
appRouter.route('/:id')

    .get(function(req,res){


        res.json(req.issue);// we wont get here if there is a 404 from above


    })
    .put(function(req,res){

        req.issue.url = req.body.url;
        req.issue.reporter = req.body.reporter;
        req.issue.title = req.body.title;
        req.issue.description = req.body.description;
        req.issue.browser = req.body.browser;
        req.issue.device = req.body.device;
        req.issue.save(function(error){ // on saving if error display error otherwise send back saved element
            if(error)
            {
                res.status(500).send(error);
            }
            else
            {
                res.json(req.issue);
            }
        });

    })
    .patch(function(req,res){
        if(req.body._id)
        {
            delete req.body._id;
        }
        for(var p in req.body) // for every key in req.body
        {
            req.issue[p] = req.body[p]; // we are assigning everything in req.issue to req.body
        }
        req.issue.save(function(error){ // on saving if error display error otherwise send back saved element
            if(error){
                res.status(500).send(error);
            }
            else
            {
                res.json(req.issue);
            }
        });
    })
    .delete(function(req,res){
       req.issue.remove(function(error){
           if(error)
           {
               res.status(500).send(error);
           }
           else
           {
               res.status(204).send('Removed'); // 204 means removed
           }
       }); // take whatever is found in middleware and remove it
    });
return appRouter;
};

module.exports = route;

Since the put delete etc for each of these is potentially going to be different it feels like I have to have a seperate route, again suggestions please

Answer 1

Your code becomes spaghetti rather easily with this approach. I suggest you move out your logic into separate modules for each route. Even cleaner, put them in a single folder so you can easily look for them. Something like this:

// app.js
var animalsModule = require('routes/animals');
var goats = require('routes/goats');

animals.route(router.route('/animals'));
goats.route(router.route('/goats'));

// animals.js
exports.route(route){
  route.get(...);
  route.post(...);
  route.put(...);
};

---

req.issue = issue;

I'm not sure if you should be altering objects you don't own, especially the request object. Doing this, req becomes your "global", a universally accessible but easily pollutable object. Removing this middleware might cause potential breakage in the future.

    req.issue.url = req.body.url;
    req.issue.reporter = req.body.reporter;
    req.issue.title = req.body.title;
    req.issue.description = req.body.description;
    req.issue.browser = req.body.browser;
    req.issue.device = req.body.device;

This code will break if the issue middleware is removed. If you write this way everywhere, you will get something like "assigning property to undefined" errors. Additionally, from what I can tell, this middleware will always run, regardless if you actually need issue or not.

I suggest you fetch issue only when needed, not all the time, and only in the location where its actually needed, not in the very available req object.

---

delete req.body._id;

There's a saying "Don't modify objects you don't own". Unless body was yours to begin with, don't be deleting properties off objects that don't come from your code. You might have unintentionally removed something that some other part of the app, a third-party module even, that needs the property.