Насколько правильно выполнено задание, что стоило бы исправить?

Question

[gleendo]

Было задание обработать запросы "GET", "POST", "DELETE".
Написал. Вроде как даже и работает все. Но явно же что тут 100500 недочетов.
Подскажите пожалуйста, что стоило бы дописать/исправить в коде?

curl http://localhost:8080/page/index.html

curl -X DELETE http://localhost:8080/page/index.html

curl -d "Hello world" http://localhost:8080/page/index.html

// app.js

let http = require("http");
let fs = require("fs");
let handler = require("./handler");

new http.createServer(handler).listen(8080);

// handler.js

let url = require("url");
let fs = require("fs");
let config = require("config");
let path = require("path");

module.exports = (req, res) => {
  let urlParsed = url.parse(req.url);
  let pathname = urlParsed.pathname;
  let filePath = path.join(config.publicRoot, pathname);

  if (!filePath.startsWith(config.publicRoot + path.sep)) {
    res.statusCode = 400;

    res.end("Bye bye silly hacker!");
  }

  fs.readFile(filePath, (err, content) => {
    if (err) {
      if (err.code == "ENOENT") {
        res.statusCode = 404;

        res.end("404");
      } else {
        res.statusCode = 500;

        res.end("Server Error")
      }
    }

    if (req.method == "GET") {
      let file = new fs.ReadStream(filePath);

      file.pipe(res);

      file.on("error", err => {
        res.statusCode = 500;
        res.end("Server error");

        console.error(err);
      });

      res.on("close", () => {
        file.destroy();
      });
    }

    if (req.method == "POST") {
      let file = new fs.createWriteStream(filePath);

      req.pipe(file);

      res.end();
    }

    if (req.method == "DELETE") {
      try {
        fs.unlink(filePath, err => {
          if (err) {
            throw err;
          }

          res.statusCode = 200;

          res.end("File was removed");
        });
      } catch (err) {
        if (err.code == "ENOENT") {
          res.statusCode = 404;

          res.end("File not found");
        } else {
          res.statusCode = 500;

          res.end("Error on removing file");
        }
      }
    }
  });
};

// config.js

let path = require("path");

module.exports = {
  projectRoot: process.cwd(),
  publicRoot: process.cwd() + path.sep + "public"
};

Comments

[rustler2000]

POST не будет работать

[gleendo]

rustler2000: Почему? У меня работает все

[rustler2000]

iamevg_: потому что он работает как PUT :D и если файла не будет, то он вернет 404

[gleendo]

rustler2000: Ну это уже другое замечание. В моем случае файл есть).
Вот первый недочет. Нужно обработать ситуацию если файла нет

[rustler2000]

Еще слишком много вложенных if и ни одного return. Помимо высокого CC это нереально легко "сломать" и крайне трудно дебажить и читать.

let filePath = path.join(config.publicRoot, pathname);

if (!filePath.startsWith(config.publicRoot + path.sep)) {

Данный if никогда не сработает )))

[gleendo]

rustler2000: А что помешает этому? Почему не сработает то?

[rustler2000]

iamevg_: а не - ../../.. может срезать - my bad

[gleendo]

rustler2000: А что такое?

высокого CC

[rustler2000]

iamevg_: https://en.wikipedia.org/wiki/Cyclomatic_complexity

Answer 1

[Николай]

Ну для начала разбить по файлам, чтобы в каждом осталось по одному хэндлеру и добавить простенький роутер, который это делает.
Следующий шаг - сделать обработку запроса как цепочку middleware, вообще по вашей формулировке задания совершенно непонятно что именно от вас хотели. На какую это вакансию хотя бы?

Comments

[gleendo]

Николай Про роутинг и middleware я еще ничего не знаю.
Это не задание при устройстве. Это д/з после первого урока видеокурса node.js (Ильи Кантора).
А задание было таким, при запросе get, отдавать файл, при post, как я понял, записывать в файл переданные данные, а при delete, удалять файл.
А что в данном коде еще является handler? Какие именно куски разбить на файлы?
Да и хотя это сейчас не так важно. Мне нужно просто понять ошибки в самом коде, правильно ли обработаны ошибки, правильные ли статус коды указаны, везде ли они указаны или еще где то нужно, правильно ли обработаны события и т.д

[Николай]

Ну тогда немного упростим:
Нужно сделать одну входную точку, которая будет иметь switch или блоки if/else, вызывающие функцию, обрабатывающую конкретный запрос(т.е. функция, которая обрабатывает GET, функция, обрабатывающая POST и функция, обрабатывающая DELETE).
По коду: довольно странная проверка с readFile, т.е. сначала проверяем что файл есть а потом в экшене ещё раз создаем стрим этого файла. Стрим и так выкинет ошибку (ту же самую ENOENT) и логичнее её ловить там.

[gleendo]

Что то типа этого?

let getHandler = require("./getHandler");
let postHandler = require("./postHandler");
let deleteHandler = require("./deleteHandler");

module.exports = (req, res) => {
	// some code

	fs.readFile(filePath, (err, content) => {
		if (err) {
			// some code
		}

		let method = req.method;

		switch (method) {
			case "GET": {
				getHandler(); break;
			}

			case "POST": {
				postHandler(); break;
			}

			case "DELETE": {
				deleteHandler(); break;
			}
		}
	});
};

[Николай]

iamevg_: ага
Только первая проверка по-хорошему должна быть уже в хендлере, а не просто перед всеми