[Moscow.pm] прошу помощи с ревью модуля
Ilya Chesnokov
chesnokov.ilya на gmail.com
Сб Ноя 29 03:03:44 PST 2014
29 ноября 2014 г., 12:03 пользователь PEF Secure <pef-secure на yandex.ru> написал:
> On Saturday, November 29, 2014 02:49:32 Ilya Chesnokov wrote:
>> Я бы все же сказал, что это ORM, т.к. он по сути отображает таблицы в
>> объекты. И даже, как я понял, автоматически определяет связи и создает
>> аксессоры для объектов из связанных таблиц.
>
> Если в этом смысле, то это не классический ORM, как я их видел. Классы
> создаются динамически исходя из структуры базы в момент коннекта или при
> первом обращении, если таблицу создали уже после того, как был коннект к базе.
Ну вот это и надо бы написать в документации в начале раздела "DESCRIPTION".
Но к слову динамическое создание схемы в памяти есть и в DBIx::Class,
но не рекомендуется из-за долгого времени загрузки в этом случае:
https://metacpan.org/pod/DBIx::Class::Schema::Loader#make_schema_at
https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#Statically-Define-Your-Schema
>> Можете конкретно объяснить, в чем его сходства и отличия от похожих
>> модулей? В чем его преимущества, недостатки перед ними, что
>> реализовано по-другому?
>> В чем преимущество, например, перед DBIx::Class - кроме разве что
>> (предположительно) меньшего потребления памяти?
>
> "Лёгкость" была главным аргументом. Легко начать использовать, создаются
> "лёгкие" объекты. Простые вещи делаются просто -- на мой взгляд уже проще
> некуда.
Ок. Думаю, тоже можно добавить в описание модуля.
>> Ничего не понял. Класс NoUser сгенерировался автоматически? Или это
>> класс исключения, который описан где-то у вас в коде? Если так, то и
>> пишите человекопонятный die() или croak().
>
> Это класс исключения, что я думал понятно по слову throw, ошибку понял.
В принципе понятно, если задуматься, но не очевидно :) Так что не
помешал бы комментарий или более привычный способ сообщения об ошибке.
>> Где комменты? Что делает, например, $client->filter_timestamp?
>
> Описано в Struct.pod.
Ок. Это не очень очевидно, поэтому не помешал бы комментарий.
>> И еще было бы неплохо показать схему таблиц, на которой вы все это
>> запускаете.
>
> Описано в Struct.pod.
Лучше было бы показать схему перед примерами, чтобы было понятнее.
>> Я не знаю, как комментировать уже существующий код на гитхабе (если
>> это не пулл реквест), поэтому отпишусь здесь.
>>
>> Во-первых, имя модуля. Я понимаю, что вы когда-то использовали (и
>> может быть используете) Class::Struct, и вам понравился его синтаксис,
>> но по-моему сходства между двумя модулями практически нет - так что не
>> вижу причины иметь Struct в названии. Впрочем, еще меньше причин иметь
>> там SQL - поскольку модуль не работает непосредственно с SQL, а скорее
>> с БД, поэтому ему больше подойдет имя вида DBIx::$something.
>
> Зависит от точки зрения. Я рассматривал модуль как предсказуемый построитель
> SQL-запросов к базе, т.е. когда я пользуюсь модулем, я представляю какой
> запрос был сгенерирован на самом деле, поэтому "SQL::". Использование "Struct"
> отражает принцип хранения данных, хотя, конечно, слабоватое основание.
Но все же модуль этот - расширение над DBI, поэтому DBIx.
Если бы он работал непосредственно с SQL, без соединения с БД, то был
бы SQL:: (может я ошибаюсь - есть ли на CPAN подобные модули,
называющиеся начиная с SQL::?).
>
>> Не заметил в коде ни одного комментария (закомментированный "#print
>> $eval_code" не считается). Я понимаю, что можно писать
>> самодокументируемый код, но это явно не ваш случай - код написан
>> сплошняком, много повторяющихся конструкций (напр.: (index($table, "
>> ") == -1)), которые неплохо бы выделить в подпрограммы.
>> Все это затрудняет понимание кода - как в процессе ревью, так и при
>> попытке что-то изменить / исправить баг и т.д. (к слову, sub
>> setup_row() - это вообще один большой пиздец).
>
> Я согласен про код и setup_row() особенно. Моя проблема в том, что я не могу
> себе представить потенциального читателя моего кода, а лично для меня не
> представляет проблемы его прочтение и через несколько лет.
"Когда пишете код, представляйте, что следующий, кто будет его
поддерживать - сумасшедший маньяк, который знает, где вы живете" :)
> Комментарии имеет
> смысл писать в уже статичном коде, в динамично меняющемся коде они могут
> устаревать и вводить в заблуждение.
Комментарии нужно писать постоянно и менять с изменением кода. Или
писать самодокументированный код: например, вместо
$client->filter_timestamp писать
$client->strip_microseconds_from_timestamp_columns (я не предлагаю это
делать - только привожу пример, как это могло бы выглядеть более
читабельно).
> Но моя проблема в том, что после
> стабилизации одного кода, я перехожу к следующему коду, оставляя написанный
> без комментариев :) Для представляемого на обсуждение кода нужно подумать как
> поменять привычки.
>
>> Не вижу документации на параметры импорта.
>
> Её нет. Не написал пока. Это вобщем новая функциональность, документацию
> добавлю.
>
>> К слову, о самой функции импорта:
>
>>
>> > sub import {
>> >
>> > my $defconn = 0;
>> > for (my $i = 1; $i < @_; ++$i) {
>> >
>> > if ($_[$i] eq 'connector') {
>> >
>> > (undef, $connector_module) = splice @_, $i, 2;
>> > --$i;
>> > if (not $defconn
>> >
>> > and check_package_scalar($connector_module, 'conn'))
>> >
>> > {
>> >
>> > *conn = \${ $connector_module . '::conn' };
>> >
>> > }
>> >
>> > } elsif ($_[$i] eq 'constructor') {
>> >
>> > (undef, $connector_constructor) = splice @_, $i, 2;
>> > --$i;
>>
>>
>> - имхо, изврат - почему бы не сделать my %arg = @_; и дальше
>> проверять, что там в %arg? Так ли уж нужен вам этот
>>
>> > $_[0]->export_to_level(1, @_);
>>
>> в конце?
>
> Он нужен, чтобы отработал Exporter.
Это понятно, я скорее имел в виду, нужен ли там последний параметр -
@_? Насколько я понял, вы из него уже вытащили все вам необходимое.
> Можно, конечно, всё руками экспортировать,
> а можно через Exporter, я выбрал последний вариант. Вернее, я не избавился от
> него после добавления кастомного варианта import(). На счёт %args -- это
> сломает вариант use SQL::Struct qw($conn).
Не заметил варианта такого использования. Ок, если так.
>> Об ошибках:
>>
>> croak \%error - это далеко не самый удобный для пользователя способ
>> получать сообщение об ошибке. Что он увидит на экране,
>> HASH(0x2558cb8)? В качестве одного из наиболее гибких способов
>> уведомления об ошибках можно рассмотреть модуль
>> https://metacpan.org/pod/Lexical::Failure.
>> Еще об ошибках: у вас везде { result => 'SQLERR' } - может в таком
>> случае вообще избавиться от поля result?
>
> Да, ошибки немного "оригинально" сообщаются. Возможно имеет смысл добавть
> встраиваемый механизм. Я ловлю и проверяю их на ref $@, в структурированном
> сообщении больше полезной информации.
Сам по себе механизм неплох, но не очень привычен для CPAN-модуля.
К слову о пространствах имен - вы генерируете классы DBC::* и DBQ::* -
где гарантия, что в коде не будет уже классов с таким именем? Может
стоит добавить имя базового модуля в качестве префикса - т.е. что-то
типа SQL::Struct::DB[C|Q]::*?
>> Да, и зачем croak(), если die() имеет тот же эффект с хешами?
>
> Привычка, да и само слово не означает "умереть" в буквальном смысле.
Зато die() побыстрее.
>> О коде:
>> > sub connect {
>> > goto &_not_yet_connected;
>> > }
>>
>>
>> А зачем такой изврат? Почему бы не сделать просто что-то типа return
>> state $connection ||= _connect();?
>
> Личное предпочтение. Именно шаблон предлагаемого кода я считаю
> "технологическим извратом", а подмену функции на возвращающую значение
> безусловно считаю "умно".
Это безусловно "умно", а следовательно нарушает принцип KISS.
>> По моему нескромному мнению для подобного гораздо лучше подходит
>> here-document.
>
> Ещё лучше подошёл бы набор шаблонов, но требовать шаблонизатор для подобного
> модуля уже перебор :) Особой разницы не вижу, но мне не принципиально.
Шаблоны - это точно перебор, а heredoc - самое то. Плюс Sub::Quote.
>>
>> ----
>>
>> Ну в-общем как-то так.
>>
>> По моему мнению модуль для публикации на CPAN не готов.
>
> Это, вобщем, я и ожидал.
Не стоит это воспринимать как приговор - это всего лишь первое ревью :)
>> Инглиш вполне сносный. Лучше напишите на русском так, чтобы все было
>> понятно. А на английский потом перевести нетрудно. Заодно и
>> презентацию можно будет сделать потом при желании.
>
> На самом деле, этот модуль, в общем-то, "мелочь", на которой я хотел
> опробовать свои навыки в публикации кода для широкой общественности.
Отлично. Исправляйте основные проблемы и публикуйте.
По большому счету единственная серьезная проблема - это 30-секундное
ожидание при отсутствии соединения с базой. Также нужно решить вопрос
об имени модуля и пространстве имен генерируемых классов. Остальное в
порядке "желательного". Даже error reporting можно оставить как есть -
только описать его в документации.
Да, еще заметил:
> $conn = $connector_module->$connector_constructor(@$connector_args)
> or croak {
> answer => "SQL_connect error",
> result => 'SQLERR',
> };
- везде 'message', а тут 'answer' - ошибка?
> Особой
> какой-то презентации и продвижения его не планировал. Мне достаточно, чтобы в
> будущем можно было делать apt-get install libsql-struct-perl (впрочем, о
> названии ещё подумаю) в своём дистрибутиве, это всё, что мне нужно.
Для своего дистрибутива можно создать отдельный репо, как многие делают.
> --
> PEF Developer
> --
> Moscow.pm mailing list
> moscow-pm на pm.org | http://moscow.pm.org
--
Best regards,
Ilya Chesnokov
Подробная информация о списке рассылки Moscow-pm