Is this nodejs web-application spaghetti code?

Question

I recently interviewed for a backend developer role at a startup involved in financial products, as a 2020 grad. The take-home assignment they had me submit had a few basic goals and they recommended the Nodejs/MongoDB stack. I received the following feedback on my submission:

1. Visibility of REST architecture is less.

2. Code structuring could have been better (I agree with this).

3. If the language chosen is Nodejs, at least the basic syntax usage should be correct. The opposite was observed.

My questions about this feedback are:

1. Isn't the structure of a REST API highly subjective? How can I make my application more compliant with REST goals?

2. What is "incorrect syntax usage?" If my syntax were incorrect, the project would misbehave or not work, wouldn't it? I posted this question on the r/codereview subreddit, and got little useful feedback apart from one comment saying "spaghetti." I'd love it if you could give me pointers on how to improve my code.

I'd love to learn from this exercise and am open to all feedback/criticism. My code resides at github.com/utkarshpant/portfolio-api along with the documentation. I tested the application using Postman and as an exercise, I am writing unit/integration tests for it.

I'm reproducing below a portion of the routes/portfolio_v2.js file, where I have implemented the majority of the endpoints:

const dbDebugger = require('debug')('app:db');
const apiDebugger = require('debug')('app:api');
const express = require('express');
const router = express.Router();
const Joi = require('joi');
const customError = require('http-errors');
const errorHandlerMiddleware = require('../middleware/errorHandlerMiddleware');

// importing models;
const Portfolio = require('../models/portfolioModel');

// import validations;
const validations = require('../validations/validateRequest');

// get returns on portfolio
router.get('/getReturns/:portfolioName', errorHandlerMiddleware((req, res) => {

    
    const portfolioName = req.params.portfolioName;

    (async () => {
        const portfolio = await Portfolio.findOne({ "name": portfolioName }).catch(err => res.send(error));
        
        if (!portfolio) {
            return res.status(404).send("No portfolio found");
        }

        const currentPrice = 100;
        let returns = 0;
        portfolio.securities.forEach(security => {
            apiDebugger(`The returns on ${security.ticker} are ${((currentPrice - security.avgBuyPrice) * security.shares)}`);
            returns += ((currentPrice - security.avgBuyPrice) * security.shares)
        })
        console.log("Returns:\t", returns);

        res.send({
            portfolio: portfolio.name,
            cumulativeReturns: returns
        });
    })();
}));

// place a buy trade
router.post('/buy/:portfolioName', errorHandlerMiddleware((req, res) => {
    /*
        Request body includes:
        Trade object, including ticker, type, quantity, price
        TODO:
        - validations for 
            - ticker match
            - trade type == sell,
            - shares - quantity > 0 always
            - resultant shares > 0 always
    */

    // validating request body;
    const { error } = validations.validateTradeRequest(req);
    if (error) {
        throw customError(400, "Bad Request; re-check the request body.");
    } else {

        // mismatch of type;
        if (req.body.type != "buy") {
            throw customError(400, "Bad request; type must be 'buy'.")
        }

        
        const portfolioName = req.params.portfolioName;    
        const trade = req.body;
        (async () => {
            // retrieve portfolio and find relevant security;
            const portfolio = await Portfolio.findOne({ "name": portfolioName }).catch(err => res.send(err));

            if (!portfolio) {
                return res.status(404).send("No portfolio found");
            }

            const security = portfolio.securities.find(security => security.ticker == trade.ticker);

            // if the ticker does not exist, return a 404;
            if (!security) {
                return res.status(404).send("Invalid ticker.");
            }
            // register sell trade and update shares;
            security.trades.push(trade);
            let oldShares = security.shares;
            security.shares += trade.quantity;
            security.avgBuyPrice = (((security.avgBuyPrice) * (oldShares)) + ((trade.price) * (trade.quantity))) / (security.shares);

            apiDebugger(`(security.avgBuyPrice): ${security.avgBuyPrice},\nsecurity.shares: ${security.shares},\ntrade.price: ${trade.price},\ntrade.quantity: ${trade.quantity}\n`);

            // save portfolio
            try {
                await portfolio.save().then(res.status(200).send(trade));
            } catch (err) {
                let errorMessages = [];
                ex.errors.forEach(property => errorMessages.push(property));
                res.status(500).send("An error occured in saving the transaction.")
            }
        })();
    }
}));

// place a sell trade
router.post('/sell/:portfolioName', errorHandlerMiddleware((req, res) => {
    /*
        Request body includes:
        Trade object, including ticker, type, quantity
        TODO:
        - validations for 
            - ticker match
            - trade type == sell,
            - shares - quantity > 0 always
            - resultant shares > 0 always
    */
    // validating request body;
    const { error } = validations.validateTradeRequest(req);
    if (error) {
        throw customError(400, "Bad Request; re-check the request body.");
    } else {
        if (req.body.type != "sell") {
            throw customError(400, "Bad Request; type must be 'sell'.");
        }


        const portfolioName = req.params.portfolioName;    
        const trade = req.body;
        (async () => {
            // retrieve portfolio and find relevant security;
            const portfolio = await Portfolio.findOne({ "name": portfolioName }).catch(err => res.send(err));

            if (!portfolio) {
                return res.status(404).send("No portfolio found");
            }

            const security = await portfolio.securities.find(security => security.ticker == trade.ticker);

            // check that resultant share count > 0;
            if ((security.shares - trade.quantity) < 0) {
                // throw customError(422, `The given Trade will result in ${security.shares - trade.quantity} shares and cannot be processed.`);
                return res.status(422).send(`Request cannot be serviced. Results in (${security.shares - trade.quantity}) shares.`);
            }

            // register sell trade and update shares;
            security.trades.push({ "ticker": trade.ticker, "type": "sell", quantity: trade.quantity });
            security.shares -= trade.quantity;

            // save portfolio
            try {
                await portfolio.save().then(res.status(200).send(trade)).catch();
            } catch (err) {
                let errorMessages = [];
                ex.errors.forEach(property => errorMessages.push(property));
                res.status(500).send("An error occured in saving the transaction.")
            }
        })();
    }
}));


function validateRequest(request) {
    const tradeRequestSchema = Joi.object({
        ticker: Joi.string().required().trim(),
        type: Joi.string().required().valid("buy", "sell").trim(),
        quantity: Joi.number().required().min(1).positive(),
        price: Joi.number().min(1).positive()
    })

    return tradeRequestSchema.validate(request.body);
}

module.exports = router;

Thanks!

Answer 1

From a medium review;

• The exception handling definitely raises eyebrows

  • ex is not defined, so ex.errors.forEach(property => errorMessages.push(property)); will fail

  • errorMessages is a mystery, you don't seem to do anything with it?

  • This

            let errorMessages = [];
            ex.errors.forEach(property => errorMessages.push(property));
    

    should be

            const errorMessages = err.errors;
    

  • Similarly, .catch(err => res.send(error)); is not going to work

• Use a hint tool like https://jshint.com/

• async () => creates an anymous function, you should use named functions for stack trace reasons

• Wrap console.log in some kind of verbosity level filtering function, never write to console.log directly

• The calculation of returns would be perfect to show off reduce()

• The big comment of router.post('/buy/:portfolioName' should be above the function

• To me :portfolioName/buy is much more REST like than '/buy/:portfolioName', I look at the big players and they tend to go much more for noun/verb then verb/noun

• If I were to log errors for a trading app, I would

  1. Create for each error a GUID, and dump that GUID in to server logs
  2. Als provide that GUID in the 500 message, "An error occured in saving the transaction." is so devoid of information, it could get you fired

• When I see this;

   if (error) {
       throw customError(400, "Bad Request; re-check the request body.");
   } else {
  

  • I cringe, why do you else after a throw ?
  • why do you not do else after the next throw ?

• Was it their design to check for type even if the type is part of the URL? That's just bad design

• You should have wrapped validateTradeRequest in a function that

  1. Calls validateTradeRequest
  2. Checks the type
  3. Checks the portfolio type
  4. Throws an error if anything else goes wrong
  5. Re-used this function in both sell and buy

• return res.status(404).send("Invalid ticker."); is an unfortunate choice of http error code and message. REST wise, 404 means that the portfolio does not exist, but it does exist. Message wise, imagine if the message said No security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• One time you use portfolio.securities.find and the other time await portfolio.securities.find?? I stopped reviewing in depth here..

• As for visibility, I agree with the reviewer, imagine that your code started with

   router.get( '/:portfolio/getReturns', getPortfolioReturns);
   router.post('/:portfolio/buy', postPortfolioBuy);
   router.post('/:portfolio/sell', postPortfolioSell);
  

  the reader would know in 1 split second what this code does (possibly be annoyed by that one space to align the get with the post ;)) Now the reader has to slog through lines and lines of code to figure this out.

• For the syntax part, your use of async triggers my spidey sense, but every time I think I found something I am wrong. Either the reviewer is really smart, or wants to appear really smart

All in all, and I rarely say this, this should have had tests. More specifically this should have had tests for failures, and you would have found and fixed a lot of these problems.