Blog API implementation in node.js

Question

This code is from my blog project. This project is almost done. This is working well. I can create new posts and update and display all saved posts.

const express = require("express");
const router = express.Router();
const Article = require("../models/BlogPost.model");

router.get("/", (req, res) => {
    res.redirect("/");
});

code for new article

router.get("/new", (req, res) => {
    res.render("new", { post: new Article() });
});

router.post("/new", (req, res) => {
    let post = new Article({
        title: req.body.title,
        author: req.body.author,
        post: req.body.post,
});
post.save((err, data) => {
    if (err) {
        res.render("posts", { post: post });
    } else {
        res.render("mypost", { result: data });
    }
});
});

Find a certain id article in database and update that article

router.get("/edit/:id", async (req, res) => {
    await Article.findById(req.params.id, (err, data) => {
        if (err) {
            res.send("error occured" + err);
        } else {
            res.render("edit", { post: data });
        }
        });
});

router.post("/edit/:id", async (req, res) => {
    try {
        await Article.findOneAndUpdate(
            { _id: req.params.id },
            { title: req.body.title, author: req.body.author, post: req.body.post },
            { new: true })
            .then((data) => {
                res.render("mypost", { result: data });
                });
            } catch (error) {
                console.log("Error found " + error);
            }
            });

router.get("/view/:id", async (req, res) => {
    await Article.findById(req.params.id, (err, data) => {
        if (err) {
            res.send("error occured" + err);
        } else {
            res.render("mypost", { result: data });
        }
    });
});

module.exports = router;

I want my code to reviewed and some tips like how to write better code. Formatted by https://st.elmah.io

Answer 1

Usage of async and await

It appears that async and await are being used but not fully utilized. Typically when using await the return value can be stored in a variable instead of using a callback handler (e.g. in a chained .then()). For instance:

router.post("/edit/:id", async (req, res) => {
    try {
        await Article.findOneAndUpdate(
            { _id: req.params.id },
            { title: req.body.title, author: req.body.author, post: req.body.post },
            { new: true })
            .then((data) => {
                res.render("mypost", { result: data });
                });
            } catch (error) {
                console.log("Error found " + error);
            }
            });

This could be updated like this:

router.post("/edit/:id", async (req, res) => {
    try {
        const data = await Article.findOneAndUpdate(
            { _id: req.params.id },
            { title: req.body.title, author: req.body.author, post: req.body.post },
            { new: true })
        res.render("mypost", { result: data });
    } catch (error) {
        console.log("Error found " + error);
    }
});

With this approach there is no need to have the .then() callback. Since I can't see the source of the BlogPost model I may be incorrect about the methods but presumably the calls can be converted as well. For example:

await Article.findById(req.params.id, (err, data) => {
    if (err) {
        res.send("error occured" + err);
    } else {
        res.render("mypost", { result: data });
    }
});

Can be instead:

const data = await Article.findById(req.params.id).catch(err => {
    res.send("error occured" + err);
});
res.render("mypost", { result: data });

This is much more succinct.

Indentation

While it could be due to copy and paste issues, the indentation is not always consistent. For example:

router.post("/new", (req, res) => {
    let post = new Article({
        title: req.body.title,
        author: req.body.author,
        post: req.body.post,
});
post.save((err, data) => {
    if (err) {
        res.render("posts", { post: post });
    } else {
        res.render("mypost", { result: data });
    }
});
});

The line before the line containing post.save() through the second to last line should be indented one level for consistent formatting.

Variable Naming

Names like data aren't very informative. A name like post or article would be more appropriate.

Variable declarations

Variable post can be declared with const instead, since it is only assigned once. This can help avoid accidental re-assignment and other bugs.

Answer 2

Since you want tips on how to write better code, I think what you're doing right now is great! Creating your own blog and asking for feedback is a great way to improve. Keep at it.

As for the code, I want to hone in on one aspect that can be quite impactful on code quality, and that is error handling. Right now you're making sure to handle errors, which is good, but notice how the error handling is obscuring the rest of the program? Our goal should be to handle errors without letting it take over the entire codebase.

I would do the following:

• Separate known failures from unknown errors. A known failure can be that the article id doesn't exist. An unknown error can be that the database is unavailable.
• Handle known failures explicitly. E.g. return 404 when a record doesn't exist. Handle unknown errors more generally. The point is that with the former you may have a reasonable strategy to deal with the failure, and with the latter you just want to not die.

So for expressjs you can wrap your routes in order to catch thrown errors, convert them to expressjs errors, and add express error middleware to handle the error (https://expressjs.com/en/guide/error-handling.html). It would look something like this:

const viewArticleByIdRoute = async (req, res) => {
    const data = await Article.findById(req.params.id)
    if (data === null) {
        return render404() // Just a dummy, fill in the blanks
    }
    res.render("mypost", { result: data })
}

const route = fn => (req, res, next) => {
    fn(req, res).catch(next)
}

router.get("/view/:id", route(viewArticleByIdRoute))

router.use((err, req, res, next) => {
    // render general error page
})

Notice how the known failure (article doesn't exist) is handled explicitly, while all other errors are just thrown and caught by the general error middleware.