Allowing users to view and manage articles

Question

This is a controller from an Express app that lets users view and manage articles. There are at least a couple of issues I'd like to address...

The program displays a list of articles in a sidebar that appears on each page. Right now, the app queries the database for article names, caches them, and includes them in the payload. It does this for each route that loads a view.

Is there a more efficient way to do this?

A good bit of code is repeated in the first two methods (retrieve and settings). Both methods get a list of articles for the sidebar and show an article or the settings for an article.

Is there a way to DRY up this code?

const { Article } = require('../models');
import CacheService from '../services/cache';

const ttl = 60 * 60 * 1;
const cache = new CacheService(ttl);

module.exports = {
  // display one article
  retrieve (req, res, next) {
    // list of articles displayed in the sidebar
    const allArticles = module.exports.getUserArticles(req.user.id);

    // the article to display
    const thisArticle = module.exports.getArticleById(req.params.publicId);

    Promise
      .all([allArticles, thisArticle])
      .then((result) => {
        const articles = result[0];
        const article = result[1];

        if (!article) {
          const error = new Error('Article not found');
          error.status = 404;
          return next(error);
        }

        return res.render('article', { articles, article, title: article.name });
      })
      .catch(next);
  },
  // show settings page for an article
  settings (req, res, next) {
    // list of articles displayed in the sidebar
    const allArticles = module.exports.getUserArticles(req.user.id);

    // the article to display
    const thisArticle = module.exports.getArticleById(req.params.publicId);

    Promise
      .all([allArticles, thisArticle])
      .then((result) => {
        const articles = result[0];
        const article = result[1];

        if (!article) {
          const error = new Error('Article not found');
          error.status = 404;
          return next(error);
        }

        const { page } = req.params;
        const pageTitle = module.exports.getSettingsPageTitle(page);
        const pageTemplate = `settings-${page}`;

        return res.render(pageTemplate, { articles, article, title: `${article.name}: ${pageTitle}` });
      })
      .catch(next);
  },
  getUserArticles (userId) {
    return cache.get(`getUserArticles_${userId}`, () => Article.findAll({ where: { userId }, order: [['name']] }));
  },
  getArticleById (publicId) {
    return cache.get(`getArticleById_${publicId}`, () => Article.findOne({ where: { publicId } }));
  },
  getSettingsPageTitle (page) {
    return {
      edit: 'Edit name, URL',
      response: 'Edit email response',
      notifications: 'Configure notifications'
    }[page];
  }
};

Answer 1

Try to envision how you would want your code to look, then build from there.

For example, I would like for the articles in the sidebar to just be there, so I pretend I have a function I can call that just fixes this for me.

res.render('article', { articles, article, title: article.name })
vs
render('article', {article, title: article.name })

Where does this new render function come from? I don't care, just take it as a dependency.

The same thing could be said about the cache and the article lookup. Just pretend it is there already.

Other than that, you could:

• Refactor to async/await
• Handle errors using a wrapper around your controller functions instead of .catch(err)
• Remove a few redundant comments
• Use a class with dependencies injected

I ended up with the following after a quick refactor:

class ArticleController{
    constructor(articleRepo){
        this._articleRepo = articleRepo
    }

    async retrieve(req, res, render){
        const article = await this._getArticleOrFail(req.params.publicId)
        return render('article', {article, title: article.name})
    }

    async settings(req, res, render){
        const article = await this._getArticleOrFail(req.params.publicId)

        const {page} = req.params
        const pageTitle = this._getSettingsPageTitle(page)
        const pageTemplate = `settings-${page}`

        return render(pageTemplate, {article, title: `${article.name}: ${pageTitle}`})
    }

    async _getArticleOrFail(id){
        const article = await this._articleRepo.getArticleById(id)

        if(!article){
            const error = new Error('Article not found')
            error.status = 404
            throw error
        }

        return article
    }

    _getSettingsPageTitle(page){
        return {
            edit: 'Edit name, URL',
            response: 'Edit email response',
            notifications: 'Configure notifications'
        }[page]
    }
}

module.exports = ArticleController

To get this working you need a couple of higher-order functions around your controller functions. One to create the render function that automatically adds articles, and one that handles errors and passes them into next(). Lastly, you need to create an ArticleRepo that you can pass as a dependency.

Some ideas:

class SidebarLayout{
    constructor(articleRepo){
        this._articleRepo = articleRepo
    }
    withSidebarData(controllerFunction){
        return async (req, res) => {
            const articles = await this._articleRepo.getUserArticles(req.user.id)
            const render = (template, data) => res.render(template, {...data, articles})

            return controllerFunction(req, res, render)
        }
    }
}

const createRoute = routeFunction => (req, res, next) => routeFunction(req, res, next).catch(next)

const articleRoute = createRoute(
    sidebarLayout.withSidebarData(articleController.retrieve.bind(articleController))
)

app.get('article', articleRoute)

There are multiple other ways to do it as well. You could have a more general templating class to inject alongside articlerepo instead of the middleware solution, or you could even decide on the actual template to use outside of the controller and simply pass in a template. But the point is to offload the necessary bits of logic outside of the controller.