1
\$\begingroup\$

I am currently creating a website for an artist (my grandfather). My main concern with the code below is readability, as for the most part I have not worked with others on a (programming) project. I use a few external modules but hopefully their purpose is obvious/irrelevant.

I have been programming with javascript as a hobby for about two years.

Here is my code:

const express = require('express')
const app = express()
const fs = require('fs')
const gzip = require('express-gzip')
const handleRequest = require('./handleRequest')
const verify = require('../Admin/login').verify

app.use(gzip)
app.use(express.json())

app.get(['/bio', '/contact', '/login', '/works', '/404', '/bug', '/edit-bio', '/edit-works'], (req, res) => verify(req.query.token).then(result => fs.readFile((req.path === '/edit-bio' || req.path === '/edit-works' ? result : true) ? ((result && req.path === '/login') ? '../../Frontend/html/edit-works.html' : `../../Frontend/html${req.path}.html`) : '../../Frontend/html/login.html', (err, data) => err ? res.status(500).end() : res.end(data))))
app.get('/', (_req, res) => fs.readFile('../../Frontend/index.html', (err, data) => err ? res.status(500).end() : res.end(data)))
app.post(/.+/, (req, res) => handleRequest[req.body.type](req.body).then(result => result.err ? res.status(500).end(JSON.stringify(result.data)) : res.end(JSON.stringify(result.data))))

app.use('/html', (_req, res) => res.status(404).redirect('/404'))
app.use(express.static('../../Frontend'))
app.use((_req, res) => (res.status(404).redirect('/404'))).listen(8080)

This is a simple express server that answers get requests on: '/bio', '/contact', '/login', etc. If the request is to '/edit-bio' or '/edit-works' it verifies a jwt provided in the query string. If it receives a post request of any kind it runs a function corresponding to a type property in that request. It also has a simple 404 page. Finally it uses express.static on a Frontend folder containing the css, js, html, etc.

\$\endgroup\$
9
  • \$\begingroup\$ What's the deal with lines of code hundreds of characters wide? That ruins the readability in many circumstances (like here). Also, usually you wouldn't manually be comparing a lot of paths in your routes. Instead, use your route definitions to focus on a specific route and then if you want to share code between routes, either use middleware or call shared functions from different route handlers. You don't typically want to see app.get([...]) with a whole bunch of separate paths for the same route and then code inside the route handler to separate out the routes. Make different routes. \$\endgroup\$
    – jfriend00
    Commented Oct 28, 2019 at 6:47
  • \$\begingroup\$ And, you can probably use express.static() in one middleware configuration for all your static routes rather than manually doing fs.readFile() and res.end() for each one separately. \$\endgroup\$
    – jfriend00
    Commented Oct 28, 2019 at 6:51
  • \$\begingroup\$ @jfriend00 are you saying that I should use express.static() and then reroute the requests to the paths to the HTML file? And then replace app.use('/html', (_req, res) => res.status(404).redirect('/404')) with app.use(['/html/edit-bio', '/html/edit-works'], (_req, res) => res.status(404).redirect('/404'))? \$\endgroup\$
    – MilesZew
    Commented Oct 28, 2019 at 23:11
  • \$\begingroup\$ Let's just start by you should increase the readability of the app.get(['/bio', '/contact', '/login', '/works', '/404', '/bug', '/edit-bio', '/edit-works'] ... line of code. Even after putting it into an editor and breaking it into multiple lines, it still isn't obvious to me what that line does except that it's got multiple checks for various paths which means you aren't taking advantage of the simplicity of using separate route declarations for separate paths. And, I can't imagine how one would add three more routes to it without a pretty good change of breaking something. \$\endgroup\$
    – jfriend00
    Commented Oct 29, 2019 at 0:09
  • \$\begingroup\$ Maybe if I have a lot more patience sometime in the future, I'll finally figure out what that line is supposed to do an rewrite it into separate routes or figure out what can just be done with express.static(). But, the MAIN point is that that line is not easily readable. Probably several different approaches to simplify it. \$\endgroup\$
    – jfriend00
    Commented Oct 29, 2019 at 0:09

1 Answer 1

Reset to default
2
\$\begingroup\$

Some suggestions:

  1. Rewrite the line of code app.get(['/bio', '/contact', '/login', '/works', '/404', '/bug', '/edit-bio', '/edit-works'] ... to be readable and maintainable and debuggable. It should likely be broken into at least two separate routes.

  2. Separate into different route definitions the paths that care about calling verify() and the ones that don't.

  3. Use express.static for routes that don't appear to have any special processing such as ['/bio', '/contact', '/login', '/works', '/404', '/bug'].

  4. Use res.sendFile() instead of repeating this err ? res.status(500).end() : res.end(data) over and over again.

  5. Put your verify logic into a middleware format so it can easily be attached to any route.

\$\endgroup\$

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.