Code review #2 | новости для мужчин

d_x умотал в Лас-Вегас просаживать годовой бюджет маленькой африканской страны, а значит, за неимением лучших тем (точнее они есть, но я лентяй), пришло время для очередного ревью. Кстати, товарищи, неужели никто не пишет на C/C++/Asm? В запросах сплошной Perl.

Начнем с кода скрипта, который прислал Alexey Shrub. Скрипт представляет из себя сокращатель ссылок и большей частью базируется на готовых модулях. Исходный код полностью. Подход нормальный, но ревьюить толком нечего ввиду отсутствия опыта использования данных модулей. Поэтому упомяну только несколько доступных мне мелочей:

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-синтаксис, который позволяет избежать потенциальных проблем при модификации подобных строковых переменных, или можно было этот фрагмент вынести в отдельный файл.

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;. Тоже непонятен смысл, переменная локальная, при выходе из зоны видимости должна уничтожаться, хотя, может, это хитрое требование используемого модуля.
И последний фрагмент из этого скрипта:

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;}

Меня интересует вторая строчка этого фрагмента. Я бы написал нечто вроде

my($key)=($req->path=~/(.+)$/);

Допустим, m использован ради того, чтобы в качестве обрамляющих символов поставить восклицательный знак (но зачем?), но целесообразность использования в данном случае [0] для меня так и осталась загадкой.

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

our$VERSION=’0.9′;…if($options{h}){ usage($VERSION);exit;}

Объявили глобальную переменную и передали её аргументом в функцию. Почему бы не обращаться к ней напрямую из функции usage? Разве что планировалось включить несколько вариантов мануалов для разных версий, но, думается, вряд ли.

if($options{i}&&!$options{s}){ simple_sintax_detect(%options);}

Не ради кода, но правильность написания слов стоит проверять хотя бы с помощью Google. Существуют syntax и sin tax. Думаю, речь шла о syntax.

sub simple_sintax_detect {my$options=shift; my$f_ext=$1if$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.

sub read_input_file {my$options=shift; if(!$options->{i}){while(<>){$msg_body.=$_;}} elsif($options->{i}){open(INFILE,'<‘,$options->{i})ordie»Can’t open $options->{i}: $!n»;while(<INFILE>){$msg_body.=$_;}close(INFILE);} return$msg_body;}

Тут мы виртуозно оперируем с глобальной переменной $msg_body, а потом её же и возвращаем зачем-то. Наверное, стоило передать её внутрь, либо объявить как локальную внутри, либо не передавать, но и не писать return $msg_body. Кстати, второй вариант чтения файла можно было реализовать более идиоматически (при условии, что мы используем вариант с return …). Примерно так:

sub read_file {local@ARGV=shift;local$/=wantarray?$/:undef;<>;}

Естественно, с адаптацией под данный случай. И последний фрагмент:

sub ua_init { my$cookies=HTTP::Cookies->new(‘file’=>’./cookies.lwp’,’autosave’=>0); my$ua= LWP::UserAgent->new(‘agent’=>’pastebinput — pastebin service agent; Этот e-mail адрес защищен от спам-ботов, для его просмотра у Вас должен быть включен Javascript ‘,’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 не используем. Тогда можно было бы ограничиться следующим кодом:

my$ua= LWP::UserAgent->new(‘agent’=>’pastebinput — pastebin service agent; Этот e-mail адрес защищен от спам-ботов, для его просмотра у Вас должен быть включен Javascript ‘,’cookie_jar’=>new HTTP::Cookies,’requests_redirectable’=>[‘GET’,’POST’],);

Последний на сегодня скрипт, а точнее модуль, присланный e.s. С листингом можно ознакомиться здесь. Этот модуль предназначен для получения доступной информации по сайту из Яндекс.Каталога (название, описание, тИЦ, рубрики…). Кода в нем мало, документация оформлена надлежащим образом, поэтому рассмотрим единственную более-менее крупную функцию — yaca_lookup, которая реализует основной функционал данного модуля. Рассматривать будет фрагментами.

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

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}=0unlessdefined$self->{_tic};}

Последние несколько строк можно было записать проще, например, следующим образом:

$self->{_tic}=$contents=~/<p class=»b-cy_error-cy»>.*?s(d+)/s?$1:0;

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, которым можно воспользоваться для отсечения захвата ненужных результатов.

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. Оставшийся код особых нареканий не вызвал.

На этом, пожалуй, завершу обзор. Если ваш код не попал в очередное ревью — не волнуйтесь, всё, что приходит, рано или поздно будет разобрано.

Также рекомендую почитать

Обсудить на форуме

Источник: http://feedproxy.google.com/~r/kaimi/dev/~3/H6UyAPX6Q3g/

Источник: lred.ru