È questo codice spaghetti dell'applicazione web nodejs?

Question

Recentemente ho sostenuto un colloquio per un ruolo di sviluppatore back-end presso una startup coinvolta in prodotti finanziari, come laureato nel 2020. Il compito da portare a casa che mi hanno fatto inviare aveva alcuni obiettivi di base e hanno consigliato lo stack Nodejs / MongoDB. Ho ricevuto il seguente feedback sulla mia richiesta:

1. La visibilità dell'architettura REST è inferiore.

2. La strutturazione del codice avrebbe potuto essere migliore (sono d'accordo con questo).

3. Se la lingua scelta è Nodejs, almeno l'uso della sintassi di base dovrebbe essere corretto. È stato osservato il contrario.

Le mie domande su questo feedback sono:

1. La struttura di un'API REST non è altamente soggettiva? Come posso rendere la mia applicazione più conforme agli obiettivi REST?

2. Che cos'è l '"utilizzo della sintassi non corretto?" Se la mia sintassi non fosse corretta, il progetto si comporterebbe male o non funzionerebbe, no? Ho pubblicato questa domanda sul subreddit r / codereview e ho ricevuto pochi feedback utili a parte un commento che diceva "spaghetti". Mi piacerebbe se potessi darmi indicazioni su come migliorare il mio codice.

Mi piacerebbe imparare da questo esercizio e sono aperto a tutti i feedback / critiche. Il mio codice risiede su github.com/utkarshpant/portfolio-api insieme alla documentazione. Ho testato l'applicazione utilizzando Postman e, come esercizio, sto scrivendo test di unità / integrazione per esso.

Sto riproducendo di seguito una porzione del routes/portfolio_v2.jsfile, dove ho implementato la maggior parte degli endpoint:

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;

Grazie!

Answer 1

Da una recensione media;

• La gestione delle eccezioni solleva sicuramente le sopracciglia

  • exnon è definito, quindi ex.errors.forEach(property => errorMessages.push(property));fallirà

  • errorMessages è un mistero, sembra che tu non ci faccia niente?

  • Questo

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

    dovrebbe essere

            const errorMessages = err.errors;
    

  • Allo stesso modo, .catch(err => res.send(error));non funzionerà

• Usa uno strumento di suggerimento come https://jshint.com/

• async () => crea una funzione anymous, è necessario utilizzare funzioni denominate per motivi di traccia dello stack

• Avvolgi console.login una sorta di funzione di filtraggio del livello di verbosità, non scrivere mai console.logdirettamente

• Il calcolo di returnssarebbe perfetto per mettersi in mostrareduce()

• Il grande commento di router.post('/buy/:portfolioName'dovrebbe essere sopra la funzione

• Per me :portfolioName/buyè molto più REST come '/buy/:portfolioName', guardo i grandi giocatori e tendono ad andare molto più per nome / verbo che per verbo / nome

• Se dovessi registrare gli errori per un'app di trading, lo farei

  1. Crea per ogni errore un GUID e scarica quel GUID nei log del server
  2. Als fornire quel GUID nel messaggio 500, "An error occured in saving the transaction."è così privo di informazioni, potrebbe farti licenziare

• Quando vedo questo;

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

  • Mi rabbrividisco, perché elsedopo un throw?
  • perchè non lo fai elsedopo il prossimo throw?

• Era il loro progetto da controllare typeanche se il tipo fa parte dell'URL? Questo è solo un cattivo design

• Avresti dovuto avvolgere validateTradeRequestin una funzione that

  1. Chiamate validateTradeRequest
  2. Controlla il tipo
  3. Controlla il tipo di portafoglio
  4. Genera un errore se qualcos'altro va storto
  5. Riutilizzata questa funzione in entrambi sellebuy

• return res.status(404).send("Invalid ticker.");è una scelta sfortunata di codice di errore http e messaggio. REST saggio, 404 significa che il portfolio non esiste, ma esiste. Per quanto riguarda il messaggio, immagina se il messaggio fosse dettoNo security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• Una volta che usi portfolio.securities.finde l'altra volta await portfolio.securities.find?? Ho smesso di rivedere in profondità qui ..

• Per quanto riguarda la visibilità, sono d'accordo con il revisore, immagina che il tuo codice sia iniziato con

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

  il lettore saprebbe in 1 frazione di secondo cosa fa questo codice (forse sarebbe infastidito da quello spazio per allinearlo getcon il post;)) Ora il lettore deve scorrere le righe e le righe di codice per capirlo.

• Per la parte di sintassi, il tuo uso di asyncinnesca il mio senso spidey, ma ogni volta che penso di aver trovato qualcosa mi sbaglio. O il revisore è davvero intelligente o vuole apparire davvero intelligente

Tutto sommato, e lo dico raramente, questo avrebbe dovuto essere sottoposto a test. Più specificamente, questo avrebbe dovuto avere test per gli errori e avresti trovato e risolto molti di questi problemi.