Est-ce le code spaghetti de l'application Web de nodejs?

Question

J'ai récemment interviewé pour un rôle de développeur backend dans une startup impliquée dans les produits financiers, en tant que diplômé 2020. Le devoir à emporter qu'ils m'ont fait soumettre avait quelques objectifs de base et ils ont recommandé la pile Nodejs / MongoDB. J'ai reçu les commentaires suivants sur ma soumission:

1. La visibilité de l'architecture REST est moindre.

2. La structuration du code aurait pu être meilleure (je suis d'accord avec cela).

3. Si le langage choisi est Nodejs, au moins l'utilisation de la syntaxe de base doit être correcte. Le contraire a été observé.

Mes questions sur ces commentaires sont:

1. La structure d'une API REST n'est-elle pas hautement subjective? Comment rendre mon application plus conforme aux objectifs REST?

2. Qu'est-ce qu'une «syntaxe incorrecte»? Si ma syntaxe était incorrecte, le projet se comporterait mal ou ne fonctionnerait pas, n'est-ce pas? J'ai posté cette question sur le sous-répertoire r / codereview , et j'ai reçu peu de commentaires utiles à part un commentaire disant "spaghetti". J'adorerais si vous pouviez me donner des conseils sur la façon d'améliorer mon code.

J'adorerais apprendre de cet exercice et je suis ouvert à tous les commentaires / critiques. Mon code se trouve sur github.com/utkarshpant/portfolio-api avec la documentation. J'ai testé l'application avec Postman et en guise d'exercice, j'écris des tests unitaires / d'intégration pour celle-ci.

Je reproduis ci-dessous une partie du routes/portfolio_v2.jsfichier, où j'ai implémenté la majorité des points de terminaison:

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;

Merci!

Answer 1

D'un examen moyen;

• La gestion des exceptions soulève définitivement les sourcils

  • exn'est pas défini, donc ex.errors.forEach(property => errorMessages.push(property));échouera

  • errorMessages est un mystère, vous ne semblez rien y faire?

  • Cette

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

    devrait être

            const errorMessages = err.errors;
    

  • De même, .catch(err => res.send(error));ne fonctionnera pas

• Utilisez un outil de conseil comme https://jshint.com/

• async () => crée une fonction anymous, vous devez utiliser des fonctions nommées pour des raisons de trace de pile

• Enveloppez console.logdans une sorte de fonction de filtrage du niveau de verbosité, n'écrivez jamais console.logdirectement

• Le calcul de returnsserait parfait pour montrerreduce()

• Le gros commentaire de router.post('/buy/:portfolioName'devrait être au-dessus de la fonction

• Pour moi, :portfolioName/buyc'est beaucoup plus REST que '/buy/:portfolioName', je regarde les gros joueurs et ils ont tendance à aller beaucoup plus pour le nom / verbe que pour le verbe / nom

• Si je devais enregistrer des erreurs pour une application de trading, je le ferais

  1. Créez pour chaque erreur un GUID et videz ce GUID dans les journaux du serveur
  2. Als fournit que GUID dans le message 500, "An error occured in saving the transaction."est tellement dépourvu d'informations, il pourrait vous faire virer

• Quand je vois cela;

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

  • Je grince des dents, pourquoi as-tu elseaprès un throw?
  • pourquoi ne faites-vous pas elseaprès le suivant throw?

• Était-ce leur conception à vérifier typemême si le type fait partie de l'URL? C'est juste une mauvaise conception

• Vous devriez avoir enveloppé validateTradeRequestdans une fonction qui

  1. Appels validateTradeRequest
  2. Vérifie le type
  3. Vérifie le type de portefeuille
  4. Lève une erreur si quelque chose ne va pas
  5. Réutilisé cette fonction à la fois dans selletbuy

• return res.status(404).send("Invalid ticker.");est un choix malheureux de code d'erreur http et de message. REST sage, 404 signifie que le portefeuille n'existe pas, mais il existe. Message sage, imaginez si le message ditNo security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• Une fois que vous utilisez portfolio.securities.findet l'autre fois await portfolio.securities.find?? J'ai arrêté de réviser en profondeur ici.

• En ce qui concerne la visibilité, je suis d'accord avec le réviseur, imaginez que votre code a commencé par

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

  le lecteur saurait en 1 fraction de seconde ce que fait ce code (peut-être être ennuyé par cet espace pour aligner le getavec le post;)) Maintenant, le lecteur doit parcourir des lignes et des lignes de code pour comprendre cela.

• Pour la partie syntaxique, votre utilisation de asyncdéclenche mon sens spidey, mais à chaque fois que je pense que j'ai trouvé quelque chose, je me trompe. Soit le critique est vraiment intelligent, soit veut paraître vraiment intelligent

Dans l'ensemble, et je dis rarement cela, cela aurait dû subir des tests. Plus précisément, cela aurait dû avoir des tests pour les échecs, et vous auriez trouvé et résolu un grand nombre de ces problèmes.