Ist dies der Spaghetti-Code der Webanwendung von nodejs?

Question

Ich habe kürzlich ein Interview für eine Backend-Entwicklerrolle bei einem Startup, das sich mit Finanzprodukten befasst, als 2020-Absolvent geführt. Die Take-Home-Aufgabe, die ich einreichen musste, hatte einige grundlegende Ziele und sie empfahlen den Nodejs / MongoDB-Stack. Ich habe folgendes Feedback zu meiner Einreichung erhalten:

1. Die Sichtbarkeit der REST-Architektur ist geringer.

2. Die Code-Strukturierung hätte besser sein können (da stimme ich zu).

3. Wenn die ausgewählte Sprache Nodejs ist, sollte zumindest die grundlegende Syntaxverwendung korrekt sein. Das Gegenteil wurde beobachtet.

Meine Fragen zu diesem Feedback sind:

1. Ist die Struktur einer REST-API nicht sehr subjektiv? Wie kann ich meine Anwendung mit den REST-Zielen kompatibler machen?

2. Was ist "falsche Syntaxverwendung"? Wenn meine Syntax falsch wäre, würde sich das Projekt schlecht verhalten oder nicht funktionieren, nicht wahr? Ich habe diese Frage auf dem Subreddit r / codereview gepostet und abgesehen von einem Kommentar mit der Aufschrift "Spaghetti" nur wenig nützliches Feedback erhalten. Ich würde es lieben, wenn Sie mir Hinweise geben könnten, wie ich meinen Code verbessern kann.

Ich würde gerne aus dieser Übung lernen und bin offen für alle Rückmeldungen / Kritik. Mein Code befindet sich zusammen mit der Dokumentation unter github.com/utkarshpant/portfolio-api . Ich habe die Anwendung mit Postman getestet und als Übung schreibe ich Unit- / Integrationstests dafür.

Ich reproduziere unter einem Teil der routes/portfolio_v2.jsDatei, in dem ich die meisten Endpunkte implementiert habe:

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;

Vielen Dank!

Answer 1

Aus einer mittleren Bewertung;

• Die Ausnahmebehandlung zieht definitiv die Augenbrauen hoch

  • exist nicht definiert, ex.errors.forEach(property => errorMessages.push(property));wird also fehlschlagen

  • errorMessages ist ein Rätsel, du scheinst nichts damit zu tun?

  • Diese

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

    sollte sein

            const errorMessages = err.errors;
    

  • Ebenso .catch(err => res.send(error));wird nicht funktionieren

• Verwenden Sie ein Hinweis-Tool wie https://jshint.com/

• async () => Wenn Sie eine beliebige Funktion erstellen, sollten Sie aus Gründen der Stapelverfolgung benannte Funktionen verwenden

• Schließen Sie eine console.logFilterfunktion für die Ausführlichkeitsstufe ein und schreiben Sie niemals console.logdirekt darauf

• Die Berechnung von returnswäre perfekt, um anzugebenreduce()

• Der große Kommentar von router.post('/buy/:portfolioName'sollte über der Funktion stehen

• Für mich :portfolioName/buyist es viel mehr REST als '/buy/:portfolioName', ich schaue auf die großen Spieler und sie tendieren dazu, viel mehr für Substantiv / Verb als für Verb / Substantiv zu gehen

• Wenn ich Fehler für eine Handels-App protokollieren würde, würde ich

  1. Erstellen Sie für jeden Fehler eine GUID und speichern Sie diese GUID in den Serverprotokollen
  2. Stellen Sie außerdem sicher, dass die GUID in der 500-Nachricht "An error occured in saving the transaction."so frei von Informationen ist, dass Sie möglicherweise gefeuert werden

• Wenn ich das sehe;

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

  • Ich erschrecke, warum machst du elsenach einem throw?
  • warum machst du nicht elsenach dem nächsten throw?

• War es ihr Design, zu prüfen type, ob der Typ Teil der URL ist? Das ist nur schlechtes Design

• Sie sollten validateTradeRequestin eine Funktion eingewickelt haben , die

  1. Anrufe validateTradeRequest
  2. Überprüft den Typ
  3. Überprüft den Portfolio-Typ
  4. Wirft einen Fehler, wenn etwas anderes schief geht
  5. Wieder verwendet diese Funktion in den beiden sellundbuy

• return res.status(404).send("Invalid ticker.");ist eine unglückliche Wahl des http-Fehlercodes und der Meldung. REST weise bedeutet 404, dass das Portfolio nicht existiert, aber es existiert. Stellen Sie sich vor, wenn die Nachricht lautetNo security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• Einmal benutzt du portfolio.securities.findund das andere mal await portfolio.securities.find?? Ich habe aufgehört, hier eingehend zu prüfen.

• In Bezug auf die Sichtbarkeit stimme ich dem Prüfer zu. Stellen Sie sich vor, Ihr Code begann mit

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

  Der Leser würde in 1 Sekundenbruchteil wissen, was dieser Code bewirkt (möglicherweise ärgert er sich über dieses eine Leerzeichen, um das getmit dem auszurichten post;).) Jetzt muss der Leser durch Zeilen und Codezeilen schlüpfen, um dies herauszufinden.

• Für den Syntax-Teil asynclöst Ihre Verwendung von meinen Spidey-Sinn aus, aber jedes Mal, wenn ich denke, dass ich etwas gefunden habe, liege ich falsch. Entweder ist der Rezensent wirklich schlau oder er möchte wirklich schlau erscheinen

Alles in allem, und das sage ich selten, hätte es Tests geben sollen. Genauer gesagt hätte dies Tests auf Fehler haben müssen, und Sie hätten viele dieser Probleme gefunden und behoben.