Code review #2


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

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

Мне кажется, стоило бы использовать существующий heredoc-синтаксис, который позволяет избежать потенциальных проблем при модификации подобных строковых переменных, или можно было этот фрагмент вынести в отдельный файл.

Тут мне не особо понятна логика. Окей, мы получили переменную $url, потом попытались получить хэш от неё (ну, в php вроде бы md5 от пустой строки успешно вычисляется, так что тут наверное тоже ошибки не возникнет), а далее решили провести её валидацию. Может, стоило сначала провести валидацию, а потом уже делать всё остальное? Также здесь и далее по коду используется вызов undef $w;. Тоже непонятен смысл, переменная локальная, при выходе из зоны видимости должна уничтожаться, хотя, может, это хитрое требование используемого модуля.
И последний фрагмент из этого скрипта:

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

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

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

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

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

Вместо букв "а", "я" стоило бы использовать их hex представление, чтобы избежать возможных проблем с кодировкой при правке скрипта сторонним лицом или на специфических системах. В [\wа-я-] последнюю тирешечку стоит экранировать, а список соответствий расширений файлов языкам лучше поместить в отдельный ассоциативный массив где-нибудь в начале и таким образом избавиться от кучи elsif.

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

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

Создаем объект HTTP::Cookies, хорошо, но файл для хранения указывать не имеет смысла, на мой взгляд. Пусть в памяти хранит. Далее мы передаем созданный объект в качестве значения cookie_jar для LWP::UserAgent и в дальнейшем переменную $cookies не используем. Тогда можно было бы ограничиться следующим кодом:

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

Мы получили на вход URL и хотим получить информацию о нём из Яндекс.Каталога. В данном фрагменте адрес приводится к некому каноничному для Яндекса виду. Я бы посоветовал не играться с регулярными выражениями в данном случае, а воспользоваться модулем URI::Split, тем более он входит в стандартный комплект модулей (по крайней мере, в случае с ActivePerl).

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

Далее

Исходя из undef в списке переменных, делаю предположение, что автор не в курсе non-capturing groupings, которым можно воспользоваться для отсечения захвата ненужных результатов.

Регулярные выражения для удаления HTML-разметки? А если новые возможные элементы добавят? Можно было воспользоваться HTML::Strip. Оставшийся код особых нареканий не вызвал.

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

Code review #2: 8 комментариев

  1. Парни, разобрали бы лучше толковый код. Правда Си или Асм, ну а это что - ерунда. Написали бы пакер, со степенью сжатия как у Upack, и размером таким-же - да разобрали бы - вот это было бы интересно. Фундаментальное что-нить. Пакер с высокой степенью сжатия (выше UPX), при этом имеющий небольшой размер (как FiPP 24Кб с GUI) да еще и с антиотладочными трюками! Вот это было бы действительно потрясно. А то - сокращатель ссылок. Каими, ты же очень достойный реверсер, изложи свои мысли по качеству и степени сжатия РЕ-формата. Ведь достойных пакеров, которые сжимают лучше де-фактового UPX можно на пальцах посчитать.. И все так же юзают LZMA, в чем их фишка??

    1. Что присылают, то и разбирается.
      Пакер и так присутствует http://kaimi.io/2011/12/%d1%83%d0%bf%d0%b0%d0%ba%d0%be%d0%b2%d1%89%d0%b8%d0%ba-pe-%d1%84%d0%b0%d0%b9%d0%bb%d0%be%d0%b2-exe-dll/
      Хочется особых степеней сжатия - исходный код открыт.
      LZMA используют, т.к. бесплатный открытый формат, да и реализаций полно.
      Хочется что-то там для работы с PE - d_x приедет и выложит здоровенную либу классов для этих целей.

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

      1. Так вот да, хочется чего-нить достойного для РЕ, заинтриговал - буду ждать))
        Пакер я видел - степень сжатия мала и ехешники бьет очень часто..(
        По поводу LZMA - я не к тому, и так понятно, что опенсорс. Просто к примеру Upack сжимает в разы лучше, чем UPX, хотя оба используют LZMA, вот и не пойму, откуда такая разница..(
        На счет снятия вмпрота и фимки - а зачем?? Проще де проинлайнить и все. Тем более вмпрот инлайнится не так сложно, если что - обратись к Максимусу (ну ты понял) он тебе расскажет, т.к. он действительно на этом собаку съел.
        Подскажи, как в d_x'овском пакере степень сжатия поднять? Заранее спасибо.

        1. Не знаю что он там бьет. Всякие файрфоксы, калькуляторы и т.п. без проблем пакуется и работает.
          Так возьми и разбери реализацию методов сжатия в upack или попроси у автора исходный код.
          Затем, что весь нужный код завернут в виртуальную машину, что абсолютно не радует.
          Написать базонезависимый распаковщик под предполагаемый алгоритм и написать методы упаковки, а потом ими заменить соответствующие моменты в пакере.

Добавить комментарий

Ваш адрес email не будет опубликован. Обязательные поля помечены *