¿Es este código espagueti de aplicación web de nodejs?

Question

Recientemente me entrevisté para un puesto de desarrollador de backend en una startup involucrada en productos financieros, como graduado de 2020. La tarea para llevar a casa que me hicieron enviar tenía algunos objetivos básicos y recomendaron la pila Nodejs / MongoDB. Recibí los siguientes comentarios sobre mi envío:

1. La visibilidad de la arquitectura REST es menor.

2. La estructuración del código podría haber sido mejor (estoy de acuerdo con esto).

3. Si el idioma elegido es Nodejs, al menos el uso de sintaxis básica debe ser correcto. Se observó lo contrario.

Mis preguntas sobre estos comentarios son:

1. ¿No es la estructura de una API REST muy subjetiva? ¿Cómo puedo hacer que mi aplicación cumpla mejor con los objetivos de REST?

2. ¿Qué es el "uso incorrecto de la sintaxis"? Si mi sintaxis fuera incorrecta, el proyecto se comportaría mal o no funcionaría, ¿no es así? Publiqué esta pregunta en el subreddit r / codereview y obtuve pocos comentarios útiles aparte de un comentario que decía "espaguetis". Me encantaría que me pudieran dar consejos sobre cómo mejorar mi código.

Me encantaría aprender de este ejercicio y estoy abierto a todos los comentarios / críticas. Mi código se encuentra en github.com/utkarshpant/portfolio-api junto con la documentación. Probé la aplicación usando Postman y, como ejercicio, estoy escribiendo pruebas de unidad / integración para ella.

Estoy reproduciendo a continuación una parte del routes/portfolio_v2.jsarchivo, donde he implementado la mayoría de los puntos finales:

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;

¡Gracias!

Answer 1

De una revisión mediana;

• El manejo de excepciones definitivamente levanta las cejas

  • exno está definido, por ex.errors.forEach(property => errorMessages.push(property));lo que fallará

  • errorMessages es un misterio, parece que no haces nada con él?

  • Esto

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

    debiera ser

            const errorMessages = err.errors;
    

  • Del mismo modo, .catch(err => res.send(error));no va a funcionar.

• Usa una herramienta de pistas como https://jshint.com/

• async () => crea una función de cualquier nombre, debe usar funciones con nombre por razones de seguimiento de pila

• Envuelva console.logen algún tipo de función de filtrado de nivel de verbosidad, nunca escriba console.logdirectamente

• El cálculo de returnssería perfecto para lucirsereduce()

• El gran comentario de router.post('/buy/:portfolioName'debería estar encima de la función.

• Para mí :portfolioName/buyes mucho más DESCANSO que '/buy/:portfolioName', miro a los grandes jugadores y tienden a ir mucho más por sustantivo / verbo que verbo / sustantivo

• Si tuviera que registrar errores para una aplicación comercial, lo haría

  1. Cree para cada error un GUID y vierta ese GUID en los registros del servidor
  2. También proporcionan que el GUID en el mensaje 500 "An error occured in saving the transaction."está tan desprovisto de información que podría hacer que lo despidan.

• Cuando veo esto;

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

  • Me estremezco, ¿por qué buscas elseuna throw?
  • ¿Por qué no lo haces elsedespués del siguiente throw?

• ¿Fue su diseño para verificar typeincluso si el tipo es parte de la URL? Eso es solo un mal diseño

• Deberías haberte envuelto validateTradeRequesten una función que

  1. Llamadas validateTradeRequest
  2. Comprueba el tipo
  3. Comprueba el tipo de cartera
  4. Lanza un error si algo más sale mal
  5. Re-utiliza esta función en tanto sellybuy

• return res.status(404).send("Invalid ticker.");es una elección desafortunada de código y mensaje de error http. REST sabio, 404 significa que la cartera no existe, pero existe. Mensaje sabio, imagina si el mensaje dijeraNo security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• ¿Una vez lo usas portfolio.securities.findy la otra vez await portfolio.securities.find? Dejé de revisar en profundidad aquí ..

• En cuanto a la visibilidad, estoy de acuerdo con el revisor, imagina que tu código comienza con

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

  el lector sabría en 1 fracción de segundo lo que hace este código (posiblemente se moleste por ese espacio para alinearlo getcon el post;)) Ahora el lector tiene que buscar entre líneas y líneas de código para resolver esto.

• Para la parte de sintaxis, tu uso de asyncdispara mi sentido arácnido, pero cada vez que creo que encontré algo, me equivoco. O el revisor es realmente inteligente o quiere parecer realmente inteligente

Con todo, y rara vez digo esto, esto debería haber tenido pruebas. Más específicamente, esto debería haber tenido pruebas de fallas, y habría encontrado y solucionado muchos de estos problemas.