2
\$\begingroup\$

I have put together a small node application with Express.js and jsonplaceholder.

On the homepage, it displays posts by all users. You can also filter posts by one user (author).

In app.js I have:

var createError = require('http-errors');
var express = require('express');
var axios = require('axios');
var path = require('path');
var cookieParser = require('cookie-parser');
var expressLayouts = require('express-ejs-layouts');
var logger = require('morgan');

var indexRouter = require('./routes/index');
var usersRouter = require('./routes/users');

var app = express();

global.base_url = 'https://jsonplaceholder.typicode.com';
global.title = 'Express Magazine';

// view engine setup
app.set('views', path.join(__dirname, 'views'));
app.set('view engine', 'ejs');
app.use(logger('dev'));
app.use(express.json());
app.use(express.urlencoded({ extended: false }));
app.use(cookieParser());
app.use(expressLayouts);
app.use(express.static(path.join(__dirname, 'public')));

app.use('/', indexRouter);
app.use('/users', usersRouter);

// catch 404 and forward to error handler
app.use(function(req, res, next) {
  next(createError(404));
});

// error handler
app.use(function(err, req, res, next) {
  // set locals, only providing error in development
  res.locals.message = err.message;
  res.locals.error = req.app.get('env') === 'development' ? err : {};

  // render the error page
  res.status(err.status || 500);
  res.render('error');
});

module.exports = app;

In routes\index.js:

const express = require('express');
const indexController = require('../controllers/index');
const router = express.Router();

// Get Posts
router.get('/', indexController.getHomepageData);

// Get Posts By User
router.get('/users/:uid/posts/', indexController.getPostsByUser);

// Redirect from bad routes
router.get('/users/', function(req, res) {
    res.redirect('/');
});

router.get('/users/:uid/', function(req, res) {
    res.redirect('/');
});

module.exports = router;

The indexController controler has the folwing code:

var axios = require('axios');
var helpers = require('../utils/helpers');

/* GET home page data */
exports.getHomepageData = async (req, res, next) => {
    try {
        let [userData, postData] = await Promise.all([
            axios.get(`${base_url}/users`),
            axios.get(`${base_url}/posts`)
        ]);

        const users = userData.data;
        const posts = postData.data;

        res.render('index', {
            layout: 'layout',
            pageTitle: 'All posts',
            users: users,
            currentUser: null,
            posts: posts,

        });
    } catch (error) {
        res.status(500).json(error);
    }
};

/* GET posts by user */
exports.getPostsByUser = async (req, res, next) => {
    try {
        let uid = req.params.uid;

        let [userData, currentUserData, postData] = await Promise.all([
            axios.get(`${base_url}/users`),
            axios.get(`${base_url}/users/${uid}`),
            axios.get(`${base_url}/posts?userId=${uid}`)
        ]);

        const users = userData.data;
        const currentUser = currentUserData.data;
        const posts = postData.data;

        res.render('index', {
            layout: 'layout',
            users: users,
            posts: posts,
            currentUser: currentUser,
            pageTitle: `Posts by ${currentUser.name}`,
        });
    } catch (error) {
        res.status(500).json(error);
    }
};

In the index.ejs view I use the above finctions. Sample:

<% if (posts) {%>
  <div class="row post-grid">
    <% posts.forEach(function(post) { %>
    <div class="col-sm-6 post">
      <div class="post-container">
        <h2 class="display-4 post-title"><%= post.title.toTitleCase() %></h2>
        <div class="short-desc">
          <%= post.body.capitalizeSentence() %>
        </div>
      </div>
    </div>
    <% }); %>
  </div>
<% } %>

The application works, but I am certain there is room for improvement.

Questions

  1. Is the code DRY enough?
  2. Is it missing anything essential?
\$\endgroup\$
3
  • \$\begingroup\$ Could you please edit to include the contents of routes/users.js? and is helpers actually used by the code in indexController? If so, what for? \$\endgroup\$ Commented Oct 19, 2021 at 21:49
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ I have added the required code. \$\endgroup\$ Commented Oct 20, 2021 at 7:55
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ In fact, you can see the entire repo here, if you want. \$\endgroup\$ Commented Oct 20, 2021 at 8:06

1 Answer 1

2
\$\begingroup\$

Is the code DRY enough?

It does not seem very repetitive - looks fine to me.

One could be overly pedantic and suggest that the function inside the routes be abstracted:

// Redirect from bad routes
router.get('/users/', function(req, res) {
    res.redirect('/');
});

router.get('/users/:uid/', function(req, res) {
    res.redirect('/');
});

to:

// Redirect from bad routes
const redirectToRoot = (req, res) => { res.redirect('/'); }
router.get('/users/', redirectToRoot);    
router.get('/users/:uid/', redirectToRoot);

but that may be excess work since it only appears twice. If the common function appears 3 or more times then abstracting is typically worth it.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.