[Moscow.pm] прошу помощи с ревью модуля
Ilya Chesnokov
chesnokov.ilya на gmail.com
Пт Ноя 28 14:49:32 PST 2014
28 ноября 2014 г., 17:40 пользователь PEF Secure <pef-secure на yandex.ru> написал:
> Приветствую!
>
> Задумал опубликовать на CPAN свой модуль, предназначенный для упрощения
> доступа к данным в базе. Самое главное: он не пытается построить из себя ORM
> или ещё какую то модель,
Я бы все же сказал, что это ORM, т.к. он по сути отображает таблицы в
объекты. И даже, как я понял, автоматически определяет связи и создает
аксессоры для объектов из связанных таблиц.
Можете конкретно объяснить, в чем его сходства и отличия от похожих
модулей? В чем его преимущества, недостатки перед ними, что
реализовано по-другому?
В чем преимущество, например, перед DBIx::Class - кроме разве что
(предположительно) меньшего потребления памяти?
> он просто пытается простые запросы сделать более
> перловыми, а сложные сделать просто возможными.
Очень претенциозное высказывание, к тому же неверное: "сложный" - это
не синоним "невозможного".
> Предыдущая версия этого модуля
> у меня уже пару лет используется и доказала свою полезность.
Эта фраза почему-то сразу внушает опасения...
> Специально для
> публикации на CPAN я написал на него документацию и добавил некоторые фишки,
> что давно просились.
>
> Проблем у меня, в общем-то, две: оттестировать версию для mysql и поправить
> документацию. Поскольку у меня просто нет в хозяйстве баз, отличных от
> postresql, то я не могу оттестировать использование mysql, хотя теоретический
> код для него написал.
В идеале этот код должен находиться в подкаталоге t/ в виде тестов.
"Нет в хозяйстве баз" - это не оправдание - я понимаю, что вы
"тестируете" этот код наживую - но в идеале нужно написать тесты,
которые создадут нужные таблицы для каждого типа баз и протестируют
каждую функцию модуля.
Кстати, кроме мускуля и постгри есть и другие БД. Итоговое количество
проблем = 2 + $count_rdbms_left.
> Коротко о сути модуля:
>
> my $session = one_row("session", {session_key => $input->{session}});
> NoUser->throw("User is not logged in or does not exist")
> unless defined $session;
> my $client = $session->Client;
> $client->filter_timestamp;
> $session->expires(\"now() + interval '2 day'");
> $session->update;
> my $usd_balance = $client->refClientBalance(currency => "USD");
Ничего не понял. Класс NoUser сгенерировался автоматически? Или это
класс исключения, который описан где-то у вас в коде? Если так, то и
пишите человекопонятный die() или croak().
Где комменты? Что делает, например, $client->filter_timestamp?
И еще было бы неплохо показать схему таблиц, на которой вы все это запускаете.
Посмотрите мануал к DBIx::Class - там неплохое описание - разберетесь,
как нужно делать документацию.
> Модуль находится тут: https://github.com/pef-secure/sql-struct
Я не знаю, как комментировать уже существующий код на гитхабе (если
это не пулл реквест), поэтому отпишусь здесь.
Во-первых, имя модуля. Я понимаю, что вы когда-то использовали (и
может быть используете) Class::Struct, и вам понравился его синтаксис,
но по-моему сходства между двумя модулями практически нет - так что не
вижу причины иметь Struct в названии. Впрочем, еще меньше причин иметь
там SQL - поскольку модуль не работает непосредственно с SQL, а скорее
с БД, поэтому ему больше подойдет имя вида DBIx::$something.
Теперь к самому коду. Я обращу внимание на общие проблемы и на
несколько частных случаев.
Не заметил в коде ни одного комментария (закомментированный "#print
$eval_code" не считается). Я понимаю, что можно писать
самодокументируемый код, но это явно не ваш случай - код написан
сплошняком, много повторяющихся конструкций (напр.: (index($table, "
") == -1)), которые неплохо бы выделить в подпрограммы.
Все это затрудняет понимание кода - как в процессе ревью, так и при
попытке что-то изменить / исправить баг и т.д. (к слову, sub
setup_row() - это вообще один большой пиздец).
> If you need other database support be prepared to help me to test and debug it.
- ну-ну, ну-ну.
Идем дальше.
Не вижу документации на параметры импорта. К слову, о самой функции импорта:
> 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, @_);
в конце?
> 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?
Да, и зачем 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.
> my $filter_timestamp = qq|\t\tsub filter_timestamp {
> my \$self = \$_[0];
> if(\@_ == 1) {
> for my \$f ($timestamps) {
> \$self->[@{[_row_data]}][\$fields{\$f}] =~ s/\\.\\d+\$// if \$self->[@{[_row_data]}][\$fields{\$f}];
> }
> } else {
> for my \$f (\@_[1..\$#_]) {
> \$self->[@{[_row_data]}][\$fields{\$f}] =~ s/\\.\\d+\$// if \$self->[@{[_row_data]}][\$fields{\$f}];
> }
> }\n\t\t}\n|;
По моему нескромному мнению для подобного гораздо лучше подходит here-document.
И к слову: весьма рекомендую для кодогенерации использовать модуль Sub::Quote.
> if (@refkeys) {
> for my $rk (@refkeys) {
if не нужен.
----
Ну в-общем как-то так.
По моему мнению модуль для публикации на CPAN не готов. Его нужно еще
перевести из разряда доморощенных ускоспециализированных инструментов
в разряд широкоспециализированных. Ключевые слова: документация (в
т.ч. сравнение с другими похожими модулями и комментарии к коду),
тесты, рефакторинг, читабельность, юзебельность (адаптация для
использования другими пользователями - в частности, error reporting).
> Это у меня намечено вторым шагом. Перед публикацией на prepan была надежда
кто-нибудь мне на ошибки в английском сначала покажет :)
Инглиш вполне сносный. Лучше напишите на русском так, чтобы все было
понятно. А на английский потом перевести нетрудно. Заодно и
презентацию можно будет сделать потом при желании.
--
Best regards,
Ilya Chesnokov
Подробная информация о списке рассылки Moscow-pm