Review for YourCodeReview (ru) by AlexMcArrow · Pull Request #1 · AlexMcArrow/MetricFromFuture

AlexMcArrow · 2021-11-03T08:43:43Z

Основные изменения:

Переход на модульную архитектуру

Вопросы:

Архитектура
На текущий момент есть\будет 4 (четыре) блока модулей:
- RPC - модули внешней двунаправленной связи (чтение\запись), для реализации: получение данных из сервиса (пассивный режим), управление сервисом
- Metric - модуль система сбора значений
- Logger - модуль логирования Metric при выходе контролируемых значений из допустимых диапазонов
- Caller - модуль исходящих данных (активный режим) - чем-то схож с Logger только пишет локально в файл (в момент написания данного текста пришло понимание что Logger следует сделать "драйверов" Caller, т.к. фактически исполняется одна логика)
Перечень и параметры модулей описываются в config.json (подразумевается последующая возможность вносить правки в блок-конфигурации по средствам RPC с последующим обновлением файла config.json на диске
Интересны ваши мысли, идеи, замечания по архитектуре, системе модульности и что угодно
Механизм передачи контроля дочерним модулям src/server.js#146-210 link

$$pipe - реализован для изолированного доступа "модулей" только к разрешенным методам.

Какие есть другие варианты?

Игнорировать:

src/ext/dns.js - временно закомментирован для последующей адаптации
вызов console.log - используется для логирования всего и вся на этапе разработки

YourCodeReview · 2021-11-04T08:41:58Z

Переход на модульную архитектуру
Интересно получить общие замечания по коду и советы

Мнение о вводимой архитектуре и предложения по улучшению
Реализован механизм передачи управления основным кодом дочерним модулям - корректность текущей и другие варианты реализации

lis-dev · 2021-11-04T09:27:03Z

+}
+
+/** Read args from cmd */
+var args = Parse(process.argv);


Возможно, для парсинга аргументов имеет смысл воспользоваться готовыми библиотеками, на подобие minimist

Стараюсь не использовать внешние библиотеки или включать их технические решения в свой код.
Функция Parse взята из одной библиотеки по разбору аргументов - думаю стоит указать автора

lis-dev · 2021-11-04T09:37:53Z

+        req.write(requestJSON);
+        req.end();
+    };
+}


Код, связанный с отправкой и обработкой запроса можно несколько сократить при использовании сторонней библиотеки axios или https://www.npmjs.com/package/node-fetch .

Стараюсь не использовать внешние библиотеки или включать их технические решения в свой код

Можно узнать, почему?

Есть несколько моментов:

большой размер зависимостей и их зависимостей, что увеличивает размер билда

при формировании билда (использую webpack для формирования единого JS-файла) поподает очень много "мусора" (т.е. реально не используемого кода на который ссылаются реализации)

если не использовать билды - проблема развертывания в изолированных пром. средах

желание иметь только реально задействованный код (пример: если подтянуть lodash для использования 2-3 методов, в билд упаковывается почти весь lodash)

lis-dev · 2021-11-04T09:44:11Z

+    console.log('\t--method\t*\texecuted method')
+    console.log('')
+    return
+}


Библиотека commander.js предоставляет удобный интерфейс для вывода подобного рода сообщений (да и предоставляет больше возможностей для работы с командной строкой, чем упоминаемый ранее minimist )

Стараюсь не использовать внешние библиотеки или включать их технические решения в свой код.
Однако спасибо за подсказки в части предлагаемых библиотек

lis-dev · 2021-11-04T09:53:19Z

@@ -0,0 +1,41 @@
+// /**


Видимо, файлы src/ext/_all.js и src/ext/dns.js не нужны.

Данный файл временно закомментирован (что бы не мешался в процессе билда), но в последующем будем адаптирован

lis-dev · 2021-11-04T09:59:30Z

@@ -0,0 +1,29 @@
+const caller_drivers = require('./caller/_all')


Изменился код стайл, раньше использовался camesCase

В следующей итерации приведу стилистику кода в порядок

lis-dev · 2021-11-04T10:00:18Z

@@ -0,0 +1,29 @@
+const caller_drivers = require('./caller/_all')
+
+class CALLER {


Изменился код стайл, раньше для классов использовался PascalCase

В следующей итерации приведу стилистику кода в порядок

lis-dev · 2021-11-04T10:01:23Z

+            return true
+        }
+        return false
+    }


В конструкторе не имеет смысла возвращать boolean, он всё равно вернёт инстанс

Большое спасибо, учту данный аспект

lis-dev · 2021-11-04T10:16:59Z

+
+            default:
+                return false
+                break


break лишний

На уровне исполнения кода - да. Однако оставил для соблюдения общей конструкции switch

lis-dev · 2021-11-04T10:18:47Z

+     * @param {float} value
+     */
+    ring(metric, level, value) {
+        this.$driver.ring(metric, level, value)


$driver может быть undefined, стоит учесть или здесь или в конструкторе

При создании инстанса класса будет проверятся заполненность $driver и инстанс будет исключаться из пула как не инициализированный.
Подобное поведение можно увидеть в загрузчике RPCs

lis-dev · 2021-11-04T10:35:47Z

+     */
+    start() {
+        console.log('RPC', 'JSON', 'start', this.$dsn)
+        if (this.$active) {


Можно уменьшить вложенность

if (!this.$active) { console.log('RPC', 'JSON', 'is disabled') return } ...

Большое спасибо, не обратил внимания

lis-dev · 2021-11-04T10:43:42Z

+        if (options.methods) {
+            assert(typeof options.methods === 'object' && !Array.isArray(options.methods), 'methods must be an object')
+            const keys = Object.keys(options.methods)
+            for (let n = 0; n < keys.length; n += 1) {


for (let key of keys)

Копи-паст из (https://github.com/sangaman/http-jsonrpc-server)

lis-dev · 2021-11-04T10:57:53Z

+ * Class representing a HTTP JSON-RPC server
+ * @see https://github.com/sangaman/http-jsonrpc-server
+ */
+class RpcServer {


Лучше вынести в отдельный файл

Верное замечание, однако я стремлюсь что бы каждый модуль был самодостаточным.
Возможно в последующем сформирую секцию кода провайдеры куда и отправится данный сегмент кода для пере использования в других модулях

lis-dev · 2021-11-04T10:58:44Z

+            this.applyOptions(options)
+        }
+
+        this.server = http.createServer(reqHandler.bind(this))


reqHandler используется только внутри класса, лучше его и сделать методом класса. Остальные функции ниже - тоже

Копи-паст из (https://github.com/sangaman/http-jsonrpc-server)

lis-dev · 2021-11-04T10:59:48Z

+            jsonrpc: '2.0',
+        }
+
+        if (request.id) {


Валидацию (вместе с кодами ошибок) лучше вынести в отдельный класс

Копи-паст из (https://github.com/sangaman/http-jsonrpc-server)

lis-dev · 2021-11-04T12:18:36Z

Общие советы по коду:

перейти на современные методы работы с js (es6 и выше), использование import, await/async, классы и т.д.
использовать сторонние библиотеки
придерживаться единого код стайла
разбивать код по зонам ответственности

R1KO · 2021-11-04T16:56:29Z

+            method: 'POST'
+        }
+
+        var buffer = '';


Стоит переходить на современные стандарты и использовать let/const

Хорошее замечание, спасибо

R1KO · 2021-11-04T16:59:42Z

+    console.log('\t--p.[key]\t\tset params key \t"{}"')
+    console.log('\t--method\t*\texecuted method')
+    console.log('')
+    return


; в конце выражений в одних файлах есть везде (местами пропущены), в других их нет совсем.
В пределах проекта стоит придерживаться одного стиля кода

Спасибо, буду внимательнее

R1KO · 2021-11-04T17:04:00Z

+    }
+
+    $readConfig(path) {
+        if (fs.existsSync(path)) {


Если сделать обратную проверку и выйти из функции - вложенность станет меньше

Спасибо, не досмотрел

R1KO · 2021-11-04T17:08:24Z

+            try {
+                this.$config = JSON.parse(fs.readFileSync(path))
+            } catch (error) {
+                throw new Error(error.toString())


а в чем смысл ловить исключение и прокидывать его дальше без дополнительной логики?

В текущей реализации - логики нет 😄
Уже есть понимание вынести обработку конфигурации в модуль что бы можно было использовать разных поставщиков данных (файлы, БД, и другие хранилища)

AlexMcArrow added 4 commits November 2, 2021 11:19

New structure

5b0e8dd

Server: init, start, stop, $$pipe; RPC(json): listen using $$pipe

de7275f

add HTTP JSONRPC CLI-client

dc73d16

clear CLI: unused require

c74941c

lis-dev reviewed Nov 4, 2021

View reviewed changes

R1KO reviewed Nov 4, 2021

View reviewed changes

AlexMcArrow commented Nov 5, 2021

View reviewed changes

Comment thread src/server.js

		@@ -0,0 +1,29 @@
		const caller_drivers = require('./caller/_all')

		@@ -0,0 +1,29 @@
		const caller_drivers = require('./caller/_all')

		class CALLER {

Conversation

AlexMcArrow commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YourCodeReview commented Nov 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lis-dev Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexMcArrow Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lis-dev Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lis-dev commented Nov 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexMcArrow commented Nov 3, 2021 •

edited

Loading

lis-dev Nov 4, 2021 •

edited

Loading

AlexMcArrow Nov 4, 2021 •

edited

Loading

lis-dev Nov 4, 2021 •

edited

Loading