[Moscow.pm] прошу помощи с ревью модуля

[PEF Secure]

On Saturday, November 29, 2014 02:49:32 Ilya Chesnokov wrote:
> Я бы все же сказал, что это ORM, т.к. он по сути отображает таблицы в
> объекты. И даже, как я понял, автоматически определяет связи и создает
> аксессоры для объектов из связанных таблиц.

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

> Можете конкретно объяснить, в чем его сходства и отличия от похожих
> модулей? В чем его преимущества, недостатки перед ними, что
> реализовано по-другому?
> В чем преимущество, например, перед DBIx::Class - кроме разве что
> (предположительно) меньшего потребления памяти?

"Лёгкость" была главным аргументом. Легко начать использовать, создаются 
"лёгкие" объекты. Простые вещи делаются просто -- на мой взгляд уже проще 
некуда. 

> Очень претенциозное высказывание, к тому же неверное: "сложный" - это
> не синоним "невозможного".

Бывают, на мой взгляд, сложные случаи, когда лично мне проще перейти к прямому 
SQL.

> > Предыдущая версия этого модуля
> > у меня уже пару лет используется и доказала свою полезность.

> Эта фраза почему-то сразу внушает опасения...

Я говорил про общий интерфейс. 

> В идеале этот код должен находиться в подкаталоге t/ в виде тестов.
> "Нет в хозяйстве баз" - это не оправдание - я понимаю, что вы
> "тестируете" этот код наживую - но в идеале нужно написать тесты,
> которые создадут нужные таблицы для каждого типа баз и протестируют
> каждую функцию модуля.

Да, пожалуй. К сожалению, не видел хорошего способа узнать параметры для 
коннекта к базе, что есть у пользователя, кроме трёх переменных окружения, но 
кто ж их задаст...

> Кстати, кроме мускуля и постгри есть и другие БД. Итоговое количество
> проблем = 2 + $count_rdbms_left.

Есть, я готов к ним адаптировать код, если это кому то будет нужно.

> Ничего не понял. Класс NoUser сгенерировался автоматически? Или это
> класс исключения, который описан где-то у вас в коде? Если так, то и
> пишите человекопонятный die() или croak().

Это класс исключения, что я думал понятно по слову throw, ошибку понял.

> Где комменты? Что делает, например, $client->filter_timestamp?

Описано в Struct.pod.

> И еще было бы неплохо показать схему таблиц, на которой вы все это
> запускаете.

Описано в Struct.pod.

> Посмотрите мануал к DBIx::Class - там неплохое описание -
> разберетесь, как нужно делать документацию.

Хорошо. Документация не мой конёк, но я постараюсь.

> Я не знаю, как комментировать уже существующий код на гитхабе (если
> это не пулл реквест), поэтому отпишусь здесь.
> 
> Во-первых, имя модуля. Я понимаю, что вы когда-то использовали (и
> может быть используете) Class::Struct, и вам понравился его синтаксис,
> но по-моему сходства между двумя модулями практически нет - так что не
> вижу причины иметь Struct в названии. Впрочем, еще меньше причин иметь
> там SQL - поскольку модуль не работает непосредственно с SQL, а скорее
> с БД, поэтому ему больше подойдет имя вида DBIx::$something.

Зависит от точки зрения. Я рассматривал модуль как предсказуемый построитель 
SQL-запросов к базе, т.е. когда я пользуюсь модулем, я представляю какой 
запрос был сгенерирован на самом деле, поэтому "SQL::". Использование "Struct" 
отражает принцип хранения данных, хотя, конечно, слабоватое основание.

> 
> Теперь к самому коду. Я обращу внимание на общие проблемы и на
> несколько частных случаев.
> 

Вот за это спасибо :)

> Не заметил в коде ни одного комментария (закомментированный "#print
> $eval_code" не считается). Я понимаю, что можно писать
> самодокументируемый код, но это явно не ваш случай - код написан
> сплошняком, много повторяющихся конструкций (напр.: (index($table, "
> ") == -1)), которые неплохо бы выделить в подпрограммы.
> Все это затрудняет понимание кода - как в процессе ревью, так и при
> попытке что-то изменить / исправить баг и т.д. (к слову, sub
> setup_row() - это вообще один большой пиздец).

Я согласен про код и setup_row() особенно. Моя проблема в том, что я не могу 
себе представить потенциального читателя моего кода, а лично для меня не 
представляет проблемы его прочтение и через несколько лет. Комментарии имеет 
смысл писать в уже статичном коде, в динамично меняющемся коде они могут 
устаревать и вводить в заблуждение. Но моя проблема в том, что после 
стабилизации одного кода, я перехожу к следующему коду, оставляя написанный 
без комментариев :) Для представляемого на обсуждение кода нужно подумать как 
поменять привычки. 

> Не вижу документации на параметры импорта. 

Её нет. Не написал пока. Это вобщем новая функциональность, документацию 
добавлю.

> К слову, о самой функции импорта:

> 
> > 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). 

> >     sub execute {
> >     
> >         my ($groupby, $having, $up_conditions, $up_order, $up_limit,
> >         
> >             $up_offset);
> >         
> >         for (my $i = 0; $i < @_; ++$i) {
> >         
> >             if ($_[$i] eq '-group_by') {
> >             
> >                 (undef, $groupby) = splice @_, $i, 2;
> >                 --$i;
> 
> 
> Та же фигня.

Это намеренно, параметры могут быть именованными или позиционными. 

> Об ошибках:
> 
> croak \%error - это далеко не самый удобный для пользователя способ
> получать сообщение об ошибке. Что он увидит на экране,
> HASH(0x2558cb8)? В качестве одного из наиболее гибких способов
> уведомления об ошибках можно рассмотреть модуль
> https://metacpan.org/pod/Lexical::Failure.
> Еще об ошибках: у вас везде { result => 'SQLERR' } - может в таком
> случае вообще избавиться от поля result?

Да, ошибки немного "оригинально" сообщаются. Возможно имеет смысл добавть 
встраиваемый механизм. Я ловлю и проверяю их на ref $@, в структурированном 
сообщении больше полезной информации. 

> Да, и зачем croak(), если  die() имеет тот же эффект с хешами?

Привычка, да и само слово не означает "умереть" в буквальном смысле.

> О коде:
> > sub connect {
> >     goto &_not_yet_connected;
> > }
> 
> 
> А зачем такой изврат? Почему бы не сделать просто что-то типа return
> state $connection ||= _connect();?

Личное предпочтение. Именно шаблон предлагаемого кода я считаю 
"технологическим извратом", а подмену функции на возвращающую значение 
безусловно считаю "умно".

> > sub _connect {
> > 
> >     my ($self, @args) = @_;
> >     for (1 .. 30) {
> >     
> >         my $dbh = eval { $self->SUPER::_connect(@args) };
> >         return $dbh if $dbh;
> >         sleep 1;
> >     
> >     }
> >     die $@ if $@;
> >     die "no connect";
> > 
> > }
> 
> 
> Seriously? Ждем 30 секунд, чтобы понять, что БД упала? Такое может
> быть внутри какой-то вашей системы, но никак не на CPAN.

Да, неконфигурируемый параметр, переделаю. Проблема не в падении базы. 
Банальная проблема: апгрейд софта. Пришло, например, секьюрити-обновление для 
postgresql, приложению его апгрейд с данным коннектором пройдёт 
"незамеченным", если апгрейд уложится в 30 секунд. Все запросы, не 
использующие $dbh за пределами коннектора, просто подождут восстановления 
базы. 

> По моему нескромному мнению для подобного гораздо лучше подходит
> here-document.

Ещё лучше подошёл бы набор шаблонов, но требовать шаблонизатор для подобного 
модуля уже перебор :) Особой разницы не вижу, но мне не принципиально.

> И к слову: весьма рекомендую для кодогенерации использовать
> модуль Sub::Quote. 
> 
> >     if (@refkeys) {
> >     
> >         for my $rk (@refkeys) {
> 
> 
> if не нужен.

Да, согласен. 

> 
> ----
> 
> Ну в-общем как-то так.
> 
> По моему мнению модуль для публикации на CPAN не готов. 

Это, вобщем, я и ожидал.

> Его нужно еще
> перевести из разряда доморощенных ускоспециализированных инструментов
> в разряд широкоспециализированных. 

Да, спасибо за подмеченные места.

> Ключевые слова: документация (в
> т.ч. сравнение с другими похожими модулями и комментарии к коду),
> тесты, рефакторинг, читабельность, юзебельность (адаптация для
> использования другими пользователями - в частности, error reporting).

Ага.

> Инглиш вполне сносный. Лучше напишите на русском так, чтобы все было
> понятно. А на английский потом перевести нетрудно. Заодно и
> презентацию можно будет сделать потом при желании.

На самом деле, этот модуль, в общем-то, "мелочь", на которой я хотел 
опробовать свои навыки в публикации кода для широкой общественности. Особой 
какой-то презентации и продвижения его не планировал. Мне достаточно, чтобы в 
будущем можно было делать apt-get install libsql-struct-perl (впрочем, о 
названии ещё подумаю) в своём дистрибутиве, это всё, что мне нужно. 

-- 
PEF Developer