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/
Дайджест новых статей по интернет-маркетингу на ваш email
Новые статьи и публикации
- 2024-03-13 » Стратегии SEO на 2024 год
- 2024-03-13 » Как использовать анимацию с помощью JavaScript-библиотеки GSAP
- 2024-03-13 » Использование GSAP 3 для веб-анимации
- 2024-03-13 » Cогласование топографической съёмки с эксплуатирующими организациями
- 2024-02-19 » Теряются лиды? Как настроить сквозную аналитику
- 2024-02-17 » Мерч и IT: на что обратить внимание в 2024 году
- 2024-02-16 » Копируем с RSync: основные примеры синхронизации файлов
- 2024-02-15 » Лучшие noCode AI платформы для создания диалоговых ботов
- 2024-02-14 » Факторы ранжирования Google 2024 — исследование Semrush
- 2024-02-12 » Перенос сайта на другой хостинг
- 2024-02-05 » В России сформирован реестр хостинг-провайдеров
- 2024-02-04 » Использование SSH для подключения к удаленному серверу Ubuntu
- 2024-02-03 » Подключаемся к серверу за NAT при помощи туннеля SSH. Простая и понятная инструкция
- 2024-02-02 » Настройка CI/CD для Gitlab-репозитория: схемы и гайд по шагам
- 2024-02-01 » GitLab CI Pipeline. Запуск сценария через SSH на удаленном сервере
- 2024-01-29 » Introduction to GitLab’s CI/CD for Continuous Deployments
- 2024-01-26 » Настройка GitLab CI/CD
- 2024-01-25 » Установка shell gitlab runner
- 2024-01-25 » Установка и регистрация gitlab-runner в docker контейнере
- 2024-01-25 » Переменные Gitlab-Ci
- 2024-01-25 » Настройка CI/CD в GitLab для синхронизации проекта с веб-серверами
- 2024-01-25 » Копирование файлов scp
- 2024-01-21 » Бездепозитные бонусы от казино: обзор условий и правил использования
- 2024-01-18 » Современная обработка ошибок в PHP
- 2024-01-18 » Пример шаблона проектирования MVC в PHP
- 2024-01-18 » Мифический человеко-DevOps
- 2023-12-28 » Google подвел итоги 2023 года в поиске
- 2023-12-28 » 5 ошибок отдела продаж, из-за которых вы теряете клиентов
- 2023-12-28 » Американский суд признал монополию Google на рынках дистрибуции Android-приложений
- 2023-12-28 » Хостинг-провайдер GoDaddy перестанет оказывать услуги пользователям из России
Самый лучший человек тот, который живет преимущественно своими мыслями и чужими чувствами, самый худший сорт человека - который живет чужими мыслями и своими чувствами. Из различных сочетаний этих четырех основ, мотивов деятельности - все различие людей. Люди, живущие только своими чувствами, - это звери. Толстой Лев Николаевич - (1828-1910) - великий русский писатель. Его творчество оказало огромное влияние на мировую литературу |
Мы создаем сайты, которые работают! Профессионально обслуживаем и продвигаем их , а также по всей России и ближнему зарубежью с 2006 года!
Как мы работаем
Заявка
Позвоните или оставьте заявку на сайте.
Консультация
Обсуждаем что именно Вам нужно и помогаем определить как это лучше сделать!
Договор
Заключаем договор на оказание услуг, в котором прописаны условия и обязанности обеих сторон.
Выполнение работ
Непосредственно оказание требующихся услуг и работ по вашему заданию.
Поддержка
Сдача выполненых работ, последующие корректировки и поддержка при необходимости.