Conversation
|
🍅 Пройдено тестов 15 из 18 |
|
🍅 Пройдено тестов 19 из 26 |
|
🍅 Пройдено тестов 19 из 26 |
|
🍏 Пройдено тестов 31 из 31 |
|
@Zhigalov обрати внимание: решено доп. задание |
|
🍏 Пройдено тестов 31 из 31 |
|
@Zhigalov обрати внимание: решено доп. задание |
Zhigalov
left a comment
Zhigalov
left a comment
There was a problem hiding this comment.
👍 хорошо написал!
В некоторых местах мне кажется сложновато, можно упростить.
server-core.js
Outdated
| app.use(bodyParser.json()); | ||
|
|
||
| app.get('/messages', (req, res) => { | ||
| const query = req.query; |
There was a problem hiding this comment.
const { query } = req;Чтобы два раза слово query не писать
| } | ||
|
|
||
| delete messages[messageId]; | ||
| res.json({ status: 'ok' }); |
There was a problem hiding this comment.
Я обычно в таких случаях не возвращаю ничего в теле ответа
А повзращаю только статус код 204 No Content («нет содержимого»)
There was a problem hiding this comment.
Это из спеки к таску
Метод удаляет сообщение с заданным
idи возвращает{ "status": "ok" }.
client-core.js
Outdated
| function createQueryFrom(args) { | ||
| const query = {}; | ||
|
|
||
| if (args.from) { |
There was a problem hiding this comment.
return ['from', 'to']
.filter(...)
.reduce(..., {});Будет более "расширяемо"
| const argparse = require('argparse'); | ||
| const Forge = require('forge-di').default; | ||
|
|
||
| const forge = new Forge(); |
There was a problem hiding this comment.
Расскажи какую проблему решает Forge?
В чем потеряешь, если не будешь его использовать?
There was a problem hiding this comment.
Просто было интересно попробовать dic в js.
There was a problem hiding this comment.
Выглядит как оверхэд.
Ладно бы это популярная библиотека была...
| @@ -0,0 +1,23 @@ | |||
| 'use strict'; | |||
|
|
|||
| class CommandsExecutor { | |||
There was a problem hiding this comment.
Я обычно "стратегию" реализую через объект.
С ним легче работать, минус один уровень абстракции
В чем преимущество твоего решения?
There was a problem hiding this comment.
Я бы не сказал, что это "стратегия". Скорее, "команда" в совокупности со множеством команд, CommandsExecutor - "invoker", сервер - "reciever". Но, вообще, да, можно было бы CommandsExecutor представить как объект со свойствами, содержащими команды
utils/message-formatter.js
Outdated
| formatMessageField('FROM', message.from, colors.red) + | ||
| formatMessageField('TO', message.to, colors.red) + | ||
| formatMessageField('TEXT', message.text, colors.green, '') + | ||
| `${message.edited ? colors.grey('(edited)') : ''}`; |
There was a problem hiding this comment.
Не консистентно: проверка на verbose через if, а здесь через тернарник
utils/message-formatter.js
Outdated
| }; | ||
|
|
||
| function formatMessageField(name, value, nameFormatter, lineEnding = '\n') { | ||
| if (!value) { |
| query | ||
| }); | ||
|
|
||
| return got(url, { method, body, json: true }); |
package.json
Outdated
| "got": "8.0.1", | ||
| "mocha": "4.0.1", | ||
| "nock": "5.5.0", | ||
| "querystring": "0.2.0", |
There was a problem hiding this comment.
не нашел где эта зависимость используется
всё что не используется лучше убирать, сборка будет быстрее
|
|
||
| execute(params) { | ||
| return sendRequest(params) | ||
| .then(res => res.body.map(m => formatMessage(m, params.verbose)).join('\n\n')); |
There was a problem hiding this comment.
А что делаешь в случае ошибки? 4XX или 5XX
|
🍏 Пройдено тестов 31 из 31 |
|
@Zhigalov обрати внимание: решено доп. задание |
|
🍏 Пройдено тестов 31 из 31 |
|
@Zhigalov обрати внимание: решено доп. задание |
Zhigalov
left a comment
Zhigalov
left a comment
There was a problem hiding this comment.
Единственное замечание - много лишнего кода, от которого можно избавиться и станет только понятнее. Поскольку дедлайны поджимают - закрываю задачу.
| const argparse = require('argparse'); | ||
| const Forge = require('forge-di').default; | ||
|
|
||
| const forge = new Forge(); |
There was a problem hiding this comment.
Выглядит как оверхэд.
Ладно бы это популярная библиотека была...
|
@Zhigalov дедлайн в понедельник, вроде, так что можно и поправить |
3 participants
No description provided.