d_x умотал в Лас-Вегас просаживать годовой бюджет маленькой африканской страны, а значит, за неимением лучших тем (точнее они есть, но я лентяй), пришло время для очередного ревью. Кстати, товарищи, неужели никто не пишет на C/C++/Asm? В запросах сплошной Perl. Начнем с кода скрипта, который прислал Alexey Shrub. Скрипт представляет из себя сокращатель ссылок и большей частью базируется на готовых модулях. Исходный код полностью. Подход нормальный, но ревьюить толком нечего ввиду отсутствия опыта использования данных модулей. Поэтому упомяну только несколько доступных мне мелочей: PHP: my $index = q{ <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> <title>URL shotener</title> </head> <body> <form action="/"> <fieldset> <label for="url">Enter URL:</label> <input type="text" name="url" id="url" /> <input type="submit" value="Get short link" /> </fieldset> </form> </body> </html> }; Мне кажется, стоило бы использовать существующий heredoc-синтаксис, который позволяет избежать потенциальных проблем при модификации подобных строковых переменных, или можно было этот фрагмент вынести в отдельный файл. PHP: my $respond = shift; my $url = $query->{url}; my $url_hash = Digest::Tiger::hexhash( $url ); unless ( $url ) { my $w = $respond->([200, ['Content-Type' => 'text/html; charset=utf-8']]); $w->write( $index ); undef $w; return; } Тут мне не особо понятна логика. Окей, мы получили переменную $url, потом попытались получить хэш от неё (ну, в php вроде бы md5 от пустой строки успешно вычисляется, так что тут наверное тоже ошибки не возникнет), а далее решили провести её валидацию. Может, стоило сначала провести валидацию, а потом уже делать всё остальное? Также здесь и далее по коду используется вызов undef $w;. Тоже непонятен смысл, переменная локальная, при выходе из зоны видимости должна уничтожаться, хотя, может, это хитрое требование используемого модуля. И последний фрагмент из этого скрипта: PHP: my $respond = shift; my $key = ( $req->path =~ m!/(.+)$! )[0]; $redis->get( $key, { on_done => sub { my ( $url ) = @_; my $w = $respond->([301, ['Location' => $url]]); $w->write( "Go to $url" ); undef $w; } Меня интересует вторая строчка этого фрагмента. Я бы написал нечто вроде PHP: my ($key) = ($req->path =~ /(.+)$/); Допустим, m использован ради того, чтобы в качестве обрамляющих символов поставить восклицательный знак (но зачем?), но целесообразность использования в данном случае [0] для меня так и осталась загадкой. Следующий скрипт нам прислал некто Abironka. Полный листинг можно посмотреть тут. Это простая отправлялка вывода команды или тела файла на pastebin. В начале скрипта мы наблюдаем краткое описание и пример использования. Это можно было бы переоформить в соответствии со стандартом, описанным здесь. По сути неважно, но приятно, когда соблюдаются общепринятые стандарты. PHP: our $VERSION = '0.9'; ... if ($options{h}) { usage($VERSION); exit; } Объявили глобальную переменную и передали её аргументом в функцию. Почему бы не обращаться к ней напрямую из функции usage? Разве что планировалось включить несколько вариантов мануалов для разных версий, но, думается, вряд ли. PHP: if ( $options{i} && !$options{s} ) { simple_sintax_detect(\%options); } Не ради кода, но правильность написания слов стоит проверять хотя бы с помощью Google. Существуют syntax и sin tax. Думаю, речь шла о syntax. PHP: sub simple_sintax_detect { my $options = shift; my $f_ext = $1 if $options->{i} =~ /^[\wа-я-]+\.([a-z]{1,4})$/i; if ($f_ext =~ /^(?:txt|text|)$/i) { $options->{s} = 'text'; } elsif ($f_ext =~ /^(?:pl|cgi)$/i) { $options->{s} = 'perl'; } elsif ($f_ext =~ /^sh$/i) { $options->{s} = 'bash'; } elsif ($f_ext =~ /^php$/i) { $options->{s} = 'php'; } } Вместо букв "а", "я" стоило бы использовать их hex представление, чтобы избежать возможных проблем с кодировкой при правке скрипта сторонним лицом или на специфических системах. В [\wа-я-] последнюю тирешечку стоит экранировать, а список соответствий расширений файлов языкам лучше поместить в отдельный ассоциативный массив где-нибудь в начале и таким образом избавиться от кучи elsif. PHP: sub read_input_file { my $options = shift; if ( !$options->{i} ) { while (<>) { $msg_body .= $_; } } elsif ( $options->{i} ) { open(INFILE, '<', $options->{i}) or die "Can't open $options->{i}: $!\n"; while (<INFILE>) { $msg_body .= $_; } close(INFILE); } return $msg_body; } Тут мы виртуозно оперируем с глобальной переменной $msg_body, а потом её же и возвращаем зачем-то. Наверное, стоило передать её внутрь, либо объявить как локальную внутри, либо не передавать, но и не писать return $msg_body. Кстати, второй вариант чтения файла можно было реализовать более идиоматически (при условии, что мы используем вариант с return ...). Примерно так: PHP: sub read_file { local @ARGV = shift; local $/ = wantarray ? $/ : undef; <>; } Естественно, с адаптацией под данный случай. И последний фрагмент: PHP: sub ua_init { my $cookies=HTTP::Cookies->new('file'=>'./cookies.lwp','autosave'=>0); my $ua = LWP::UserAgent->new( 'agent' => 'pastebinput - pastebin service agent; dimio@dimio.org', 'cookie_jar' => $cookies, 'requests_redirectable' => ['GET', 'POST'], ); $ua->default_header( 'Accept' => 'text/html, application/xml;q=0.9, application/xhtml+xml, */*;q=0.1', 'Accept-Charset' => 'utf-8; *;q=0.1', 'Accept-Language' => 'ru,en-us;q=0.7,en;q=0.3', 'Accept-Encoding' => 'deflate, gzip, x-gzip, *;q=0', ); return $ua; } Создаем объект HTTP::Cookies, хорошо, но файл для хранения указывать не имеет смысла, на мой взгляд. Пусть в памяти хранит. Далее мы передаем созданный объект в качестве значения cookie_jar для LWP::UserAgent и в дальнейшем переменную $cookies не используем. Тогда можно было бы ограничиться следующим кодом: PHP: my $ua = LWP::UserAgent->new( 'agent' => 'pastebinput - pastebin service agent; dimio@dimio.org', 'cookie_jar' => new HTTP::Cookies, 'requests_redirectable' => ['GET', 'POST'], ); Последний на сегодня скрипт, а точнее модуль, присланный e.s. С листингом можно ознакомиться здесь. Этот модуль предназначен для получения доступной информации по сайту из Яндекс.Каталога (название, описание, тИЦ, рубрики...). Кода в нем мало, документация оформлена надлежащим образом, поэтому рассмотрим единственную более-менее крупную функцию - yaca_lookup, которая реализует основной функционал данного модуля. Рассматривать будет фрагментами. PHP: $address =~ s|.*?://||; # loose scheme $address =~ s|.*?(:.*?)?@||; # loose authentication $address =~ s|(\w):\d+|$1|; # loose port $address =~ s|\?.*||; # loose query $address =~ s|/$||; # loose trailing slash my $contents = get( 'http://search.yaca.yandex.ru/yca/cy/ch/' . $address ); Мы получили на вход URL и хотим получить информацию о нём из Яндекс.Каталога. В данном фрагменте адрес приводится к некому каноничному для Яндекса виду. Я бы посоветовал не играться с регулярными выражениями в данном случае, а воспользоваться модулем URI::Split, тем более он входит в стандартный комплект модулей (по крайней мере, в случае с ActivePerl). PHP: if( $contents =~ /<p class="b-cy_error-cy">/ ) { # "ресурс не описан в Яндекс.Каталоге" # It's not in the catalog, but tIC is always displayed. # Ex.: Индекс цитирования (тИЦ) ресурса — 10 ( $self->{_tic} ) = $contents =~ /<p class="b-cy_error-cy">.*?\s(\d+)/s; $self->{_tic} = 0 unless defined $self->{_tic}; } Последние несколько строк можно было записать проще, например, следующим образом: PHP: $self->{_tic} = $contents =~ /<p class="b-cy_error-cy">.*?\s(\d+)/s ? $1 : 0; Далее PHP: my( $entry ) = $contents =~ qr{(<tr>\s*<td><img.*/arr-hilite\.gif".*?</tr>)}s; ( $self->{_orderNum}, $self->{_uri}, $self->{_shortDescr}, undef, $self->{_longDescr}, $self->{_tic} ) = # $1 $2 $3 $4 $5 $entry =~ qr{<td>(\d+)\.\s*</td>.*<a href="(.*?)".*?>(.*)</a>(<div>(.*)</div>.*?)?(\d+)<}s; Исходя из undef в списке переменных, делаю предположение, что автор не в курсе non-capturing groupings, которым можно воспользоваться для отсечения захвата ненужных результатов. PHP: if( $path ) { $path =~ s{</?a.*?>|</?h1>|\n}{}gs; # remove A, H1 tags and newline $path =~ s|\x{0420}\x{0443}\x{0431}\x{0440}\x{0438}\x{043A}\x{0438} / ||; # removed "Рубрики" - it always starts with it # http://www.rishida.net/tools/conversion/ push( @{$self->{_categories}}, $path.' / '.$rubric ) if $entry; } Регулярные выражения для удаления HTML-разметки? А если новые возможные элементы добавят? Можно было воспользоваться HTML::Strip. Оставшийся код особых нареканий не вызвал. На этом, пожалуй, завершу обзор. Если ваш код не попал в очередное ревью - не волнуйтесь, всё, что приходит, рано или поздно будет разобрано. автор: Kaimi Среда, 22. Август 2012 http://kaimi.ru/2012/08/code-review-2/