Este é um código espaguete de aplicativo da web nodejs?

Question

Recentemente fui entrevistado para uma função de desenvolvedor de back-end em uma startup envolvida em produtos financeiros, como um graduado de 2020. A tarefa que eles levaram para casa tinha alguns objetivos básicos e eles recomendaram a pilha Nodejs / MongoDB. Recebi o seguinte feedback sobre o meu envio:

1. A visibilidade da arquitetura REST é menor.

2. A estruturação do código poderia ter sido melhor (concordo com isso).

3. Se o idioma escolhido for Nodejs, pelo menos o uso da sintaxe básica deve estar correto. O oposto foi observado.

Minhas perguntas sobre este feedback são:

1. A estrutura de uma API REST não é altamente subjetiva? Como posso tornar meu aplicativo mais compatível com os objetivos REST?

2. O que é "uso incorreto de sintaxe?" Se minha sintaxe estivesse incorreta, o projeto se comportaria mal ou não funcionaria, não é? Publiquei esta questão no subreddit r / codereview e recebi poucos comentários úteis além de um comentário dizendo "espaguete". Eu adoraria se você pudesse me dar dicas sobre como melhorar meu código.

Adoraria aprender com este exercício e estou aberto a todos os comentários / críticas. Meu código reside em github.com/utkarshpant/portfolio-api junto com a documentação. Testei o aplicativo usando o Postman e, como exercício, estou escrevendo testes de unidade / integração para ele.

Estou reproduzindo abaixo uma parte do routes/portfolio_v2.jsarquivo, onde implementei a maioria dos 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;

Obrigado!

Answer 1

De uma revisão média;

• O tratamento de exceções definitivamente levanta sobrancelhas

  • exnão está definido, então ex.errors.forEach(property => errorMessages.push(property));irá falhar

  • errorMessages é um mistério, você não parece fazer nada com isso?

  • Esta

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

    deveria estar

            const errorMessages = err.errors;
    

  • Da mesma forma, .catch(err => res.send(error));não vai funcionar

• Use uma ferramenta de dica como https://jshint.com/

• async () => cria uma função anymous, você deve usar funções nomeadas por motivos de rastreamento de pilha

• Envolva console.logem algum tipo de função de filtragem de nível de detalhamento, nunca escreva console.logdiretamente

• O cálculo de returnsseria perfeito para exibirreduce()

• O grande comentário de router.post('/buy/:portfolioName'deve estar acima da função

• Para mim :portfolioName/buyé muito mais como REST do que '/buy/:portfolioName', eu olho para os grandes jogadores e eles tendem a ir muito mais para substantivo / verbo do que verbo / substantivo

• Se eu fosse registrar erros para um aplicativo de negociação, eu

  1. Crie para cada erro um GUID e despeje esse GUID nos logs do servidor
  2. O Als fornece esse GUID na mensagem 500, "An error occured in saving the transaction."é tão desprovido de informações que pode fazer com que você seja demitido

• Quando eu vejo isso;

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

  • Eu me encolho, por que você elsedepois de um throw?
  • por que você não faz elsedepois do próximo throw?

• Era o design deles verificar, typemesmo se o tipo fosse parte da URL? Isso é apenas um design ruim

• Você deveria ter envolvido validateTradeRequestem uma função que

  1. Ligações validateTradeRequest
  2. Verifica o tipo
  3. Verifica o tipo de portfólio
  4. Lança um erro se algo mais der errado
  5. Reutilizou esta função em ambos sellebuy

• return res.status(404).send("Invalid ticker.");é uma escolha infeliz de código de erro e mensagem http. Em termos de REST, 404 significa que o portfólio não existe, mas existe. Mensagem sábia, imagine se a mensagem dissesseNo security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• Uma vez você usa portfolio.securities.finde outra vez await portfolio.securities.find?? Parei de revisar em profundidade aqui ..

• Quanto à visibilidade, concordo com o revisor, imagine que seu código começou com

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

  o leitor saberia em 1 fração de segundo o que esse código faz (possivelmente ficar incomodado com aquele espaço para alinhar o getcom o post;)) Agora o leitor tem que se arrastar por linhas e linhas de código para descobrir isso.

• Para a parte da sintaxe, o uso de asyncdesencadeia meu senso de aranha, mas toda vez que acho que encontrei algo estou errado. O revisor é muito inteligente ou quer parecer muito inteligente

De modo geral, e raramente digo isso, deveria haver testes. Mais especificamente, deveria haver testes para falhas, e você teria encontrado e corrigido muitos desses problemas.