Apakah ini kode spaghetti aplikasi web nodejs?

Question

Saya baru-baru ini mewawancarai peran developer backend di sebuah startup yang terlibat dalam produk keuangan, sebagai lulusan 2020. Tugas dibawa pulang yang mereka minta saya kirimkan memiliki beberapa tujuan dasar dan mereka merekomendasikan tumpukan Nodejs / MongoDB. Saya menerima umpan balik berikut pada kiriman saya:

1. Visibilitas arsitektur REST kurang.

2. Penataan kode bisa lebih baik (saya setuju dengan ini).

3. Jika bahasa yang dipilih adalah Nodejs, setidaknya penggunaan sintaks dasar harus benar. Yang sebaliknya diamati.

Pertanyaan saya tentang umpan balik ini adalah:

1. Bukankah struktur REST API sangat subjektif? Bagaimana saya dapat membuat aplikasi saya lebih sesuai dengan tujuan REST?

2. Apa yang dimaksud dengan "penggunaan sintaks yang salah?" Jika sintaks saya salah, proyek akan berperilaku buruk atau tidak berfungsi, bukan? Saya memposting pertanyaan ini di subreddit r / codereview , dan mendapat sedikit umpan balik yang berguna selain dari satu komentar yang mengatakan "spaghetti." Saya akan senang jika Anda dapat memberi saya petunjuk tentang cara meningkatkan kode saya.

Saya ingin belajar dari latihan ini dan terbuka untuk semua umpan balik / kritik. Kode saya berada di github.com/utkarshpant/portfolio-api bersama dengan dokumentasinya. Saya menguji aplikasi menggunakan Postman dan sebagai latihan, saya menulis tes unit / integrasi untuk itu.

Saya mereproduksi di bawah sebagian routes/portfolio_v2.jsfile, di mana saya telah menerapkan sebagian besar titik akhir:

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;

Terima kasih!

Answer 1

Dari ulasan menengah;

• Penanganan pengecualian pasti menaikkan alis

  • extidak ditentukan, jadi ex.errors.forEach(property => errorMessages.push(property));akan gagal

  • errorMessages adalah sebuah misteri, kamu sepertinya tidak melakukan apapun dengannya?

  • Ini

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

    seharusnya

            const errorMessages = err.errors;
    

  • Demikian pula, .catch(err => res.send(error));tidak akan berhasil

• Gunakan alat petunjuk seperti https://jshint.com/

• async () => membuat fungsi anymous, Anda harus menggunakan fungsi bernama untuk alasan pelacakan tumpukan

• Bungkus console.logdalam beberapa jenis fungsi penyaringan tingkat verbositas, jangan pernah menulis console.logsecara langsung

• Perhitungannya returnsakan sempurna untuk dipamerkanreduce()

• Komentar besar router.post('/buy/:portfolioName'harus berada di atas fungsi

• Bagi saya :portfolioName/buylebih seperti REST daripada '/buy/:portfolioName', saya melihat para pemain besar dan mereka cenderung lebih banyak menggunakan kata benda / kata kerja kemudian kata kerja / kata benda

• Jika saya mencatat kesalahan untuk aplikasi perdagangan, saya akan melakukannya

  1. Buat untuk setiap kesalahan GUID, dan buang GUID itu ke log server
  2. Juga memberikan bahwa GUID dalam 500 pesan, "An error occured in saving the transaction."begitu tanpa informasi, itu bisa membuat Anda dipecat

• Saat saya melihat ini;

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

  • Aku ngeri, kenapa kamu elsesetelah a throw?
  • kenapa tidak kamu lakukan elsesetelah selanjutnya throw?

• Apakah itu desain mereka untuk diperiksa typemeskipun jenisnya adalah bagian dari URL? Itu hanya desain yang buruk

• Anda harus membungkus validateTradeRequestfungsi itu

  1. Panggilan validateTradeRequest
  2. Periksa jenisnya
  3. Periksa jenis portofolio
  4. Melempar kesalahan jika ada yang salah
  5. Menggunakan kembali fungsi ini di selldanbuy

• return res.status(404).send("Invalid ticker.");adalah pilihan kode kesalahan http dan pesan yang tidak menguntungkan. REST bijaksana, 404 berarti portofolio tidak ada, tetapi ada. Pesan bijak, bayangkan jika pesan itu berkataNo security found for ticker ${trade.ticket} in portfolio ${portfolioName}

• Suatu saat Anda menggunakan portfolio.securities.finddan lain waktu await portfolio.securities.find?? Saya berhenti mengulas secara mendalam di sini ..

• Mengenai visibilitas, saya setuju dengan peninjau, bayangkan kode Anda dimulai dengan

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

  pembaca akan tahu dalam 1 sepersekian detik apa yang dilakukan kode ini (mungkin terganggu oleh satu spasi untuk menyelaraskan getdengan post;)) Sekarang pembaca harus bekerja keras melalui baris dan baris kode untuk mencari tahu ini.

• Untuk bagian sintaks, penggunaan Anda asyncmemicu spidey sense saya, tetapi setiap kali saya pikir saya menemukan sesuatu, saya salah. Bisa jadi peninjau itu benar-benar pintar, atau ingin tampil sangat pintar

Semua dalam semua, dan saya jarang mengatakan ini, ini seharusnya ada tes. Lebih khusus lagi, ini seharusnya memiliki tes untuk kegagalan, dan Anda akan menemukan dan memperbaiki banyak masalah ini.