Print This Post Code review #1

Воскресенье, 5. Август 2012
Раздел: Code Review, автор:


Вот и наступило время для первого обзора исходников от Kaimi и dx. Не все исходники попали в этот пост, потому что прислано их было немало. Не огорчайтесь, если не увидели своего творения, мы постараемся включить его в следующие выпуски!

Начнет review Kaimi.

Первый скрипт на Perl прислал Alexandr Alexeev. Сей скрипт показывает уведомления о новых письмах в трее. Ознакомиться с оригинальным кодом можно по этой ссылке. Оформление комментировать здесь и далее не планирую, так как сам предпочитаю так называемый Allman style, но с некоторыми модификациями, да и о вкусах не спорят. Итак, перейдем к авторскому коду:

Зачем было выделять этот фрагмент в отдельную область видимости, для меня остается загадкой, особенно учитывая небольшие размеры скрипта и отсутствие существенных объемов данных в памяти, которые освободились бы при выходе из неё. Список зависимостей можно было бы и не выносить в отдельную переменную, а написать, как сделано было в коде ниже:

Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/.
Смысл использования скобок в предпоследней строке тоже не ясен. Строчка тривиальная, да и далее по коду есть множество случаев опускания скобок в подобных ситуациях. Продолжим.

Функция decrypt_file используется во всем коде только один раз, CONFIG - глобальная константа. Не знаю, насколько целесообразно было делать генерик функцию и передавать в неё переменную в этом случае.

Объект в цикле создавать не стоит. Корректнее было бы создать его вне цикла, при этом не передавать в конструктор параметры USER и PASSWORD, а в цикле вызывать метод Connect. Хотя я и обещал не комментировать оформление, но все же стоит как-то унифицировать использование скобок и кавычек (в частности, при обращении к элементам ассоциативных массивов: то они есть, то их нет...).

Следующим рассмотрим код, который создает файлы списков для зачисления (Сбербанк, dbf) из таблиц Excel файлов (xls), присланный Даниилом Поповым. Код полностью.

Исходя из кодировки, скрипт используется под win* системами, причем в качестве консольного скрипта, следовательно, указание nix-style пути к интерпретатору не имеет особого смысла. Функция decode, импортированная из модуля Encode, в коде вообще не используется, а прагмы лучше подключать друг рядом с другом (strict, warnings, locale) чисто с эстетической точки зрения.

Обрамлять переменную в кавычки не стоило. В качестве переноса строки, как я говорил ранее, лучше бы было $/ использовать, да и в Windows \r\n используется по стандарту.

В этом фрагменте видим использование push в применении к скаляру. Работать будет, но только для версии Perl >= 5.14 (что и было указано в начале скрипта), но смысл так делать? Подошел бы обычный массив, тем более "The exact behaviour may change in a future version of Perl.".

Этот фрагмент кода дублируется несколько раз, но с разным регулярным выражением. Стоило вынести в отдельную функцию. Переменная в кавычках, и снова зачем-то использован скаляр вместо того, чтобы обычный хэш создать. А ещё этот фрагмент можно заменить чем-нибудь типа:

Комментировать использование модулей Spreadsheet::ParseExcel и XBase не буду, так как не доводилось их использовать.
Хочу сделать общий ко всем исходным кодам комментарий. Определитесь со стилем использования скобочек и кавычек. Выбирайте что-нибудь одно.

Напоследок: если ваш код предполагает обработку каких-то специальных входных данных (как последний скрипт), то прикладывайте по возможности пример этих входных данных в виде файла в соответствующем формате.

Продолжает dx.

Перейдем к ассемблеру (MASM32). Morgot B прислал сорс, который ищет файлы с заданными расширениями в указанной директории. Вот он. Начну с самого начала.

Макросы - это хорошо. Упрощает код, пишем меньше строк.

Если это простая программка предназначена исключительно для обучения, не стоит задаваться такими вопросами. В идеале было бы запрашивать расширения для поиска и стартовую директорию у пользователя, а не забивать их в код программы, но здесь это не важно.

Я так понял, это своеобразный массив указателей на строки, чтобы можно было его перебирать и сравнивать расширение очередного файла с каждым из искомых. В реальности в программе используется только переменная pext1, так не проще ли переписать так:

Такой же совет могу дать и тут - переписать это как массив dword'ов и его же использовать:

Смотрим дальше:

Нет смысла выделять 1024 байта памяти на куче. Проще и гораздо быстрее будет выделить ее на стеке:

Разумеется, чтобы сделать так, надо сначала образовать стековый фрейм, обернув весь код начиная от метки start и до invoke ExitProcess,0 в какую-то процедуру:

Если здесь мы перейдем к массиву dword'ов cext, то это выражение запишется так:

А отсюда уже легко перейти к циклу по всем переменным из массива, затолкнув их в стек, а потом вызвав wsprintf (хотя это необязательно, так как набор и количество искомых расширений фиксированы). Идем дальше:

Тут можно было использовать invoke, тем более, прототип для функции findAll написан.

Зачем все это? Функция WinAPI lstrcat всегда дописывает нуллбайт в конец строки, поэтому все действия с ручным его дописыванием лишние.

Кстати, не понял, почему структура WIN32_FIND_DATA (fd) выделена в куче (точнее, в секции данных). Это убило часть возможностей функции findAll, и она никогда не сможет работать многопоточно. Впрочем, счетчики найденных расширений находятся там же, поэтому это скорее не недостаток, а упрощение.

Я бы вывел ошибку в MessageBox, хотя это не столь важно, так как программа обучающая.

Далее я вижу практически полное дублирование кода, что, естественно, не есть хорошо. Следовало бы реорганизовать код или вынести повторяющиеся моменты в отдельную функцию.

Опять-таки, можно было бы заменить вызов через push-call на invoke. Кроме того, переменная len1, по сути, не нужна - можно было сделать push eax сразу после вызова lstrlen. Да и, чего уж там, по-хорошему в функцию GetExt надо было передать просто строку, а длину она бы посчитала сама, findAll не обязана за нее это делать. Длина потребовалась, если бы GetExt принимала какие-то двоичные данные - для них просчитать длину невозможно.

Перейдем теперь к самой функции GetExt:

Этот кусок можно было бы несколько ускорить и упростить, использовав команду scasb вместо lodsb и явного сравнения (не забыв учесть, что scasb работает с регистром edi, а не esi).

Пожалуй, это все, что я хотел сказать по данному исходному коду.

Вердикт: код не очень хороший (это простительно, так как автор, видимо, только учится), его можно сильно улучшить, есть, чему еще учиться. Из явных грехов видно несколько разных видов выделения памяти, хотя можно было бы все выделять на стеке (в данном случае, так как мы не выделяем больших объемов памяти), сделав тем самым все функции потокобезопасными; попеременное использование то макросов MASM32 вроде .if - .else, то операций вроде cmp - je (т.е. опять-таки нет единообразия, код тяжелее читается). Разные функции написаны с разными "конвенциями" вызовов - GetExt сохраняет регистры, findAll - нет. Я бы посоветовал использовать везде одно и то же общепринятое, например, stdcall: сохранять регистры esi, edi и ebx, значения передавать через стек в обратном порядке и очищать стек внутри функции, результат возвращать через eax.

Перейдем к рассмотрению следующего исходного кода. Это "контроллер в некотором Zend Framework приложении для страницы управления проектами". Так как с Zend framework'ом мне приходилось работать очень давно и в течение достаточно короткого промежутка времени, то в плане архитектуры и использования методов Zend'а никаких рекомендаций дать я не смогу. Поэтому просто посмотрим на недостатки в самом коде. Вот он полностью.

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

По этому куску кода не могу в целом ничего сказать, но немного смущает полное отсутствие проверок входящих значений и экранирования. Метод fetchAll, скорее всего, производит экранирование, но что с методом/конструктором $this->_helper->json? Вероятно, есть уязвимость XSS. Еще интересно, что будет, если $_GET['query'] будет массивом. Стоит проверить этот момент.

Фукнция isset умеет принимать несколько аргументов сразу. Этот фрагмент кода можно было написать так:

Еще я бы вместо явных проверок $_POST['orderEmail'] и $_POST['project'] использовал бы функцию empty.

Странное дублирование.

Теперь итоги. В целом код выглядит весьма неплохо. В некоторых местах отсутствуют проверки ошибок (строки 122 и 129, например). Не исключаю вариант, что они там и не нужны. Отсутствует поддержка локализации, об этом я уже говорил. Вроде бы в критическим местах даже есть защита от CSRF, но не могу точно сказать, будет ли она хорошо работать. Больше, пожалуй, добавить ничего не могу. Вызываются методы неизвестного мне фреймворка, поэтому не могу полностью оценить, насколько это эффективно и безопасно.

Вот и всё на сегодня. Присылайте новые исходники, и мы обязательно постараемся их рассмотреть. До встречи в следующем Code Review!

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


Получать обновления на почту:     

Комментариев: 15 к “Code review #1”


  1. flisk :

    Тема получилась очень интересная и полезная. Надеюсь, будет еще продолжение, т.к. реально не у кого часто спросить по качеству кода, или почитать советы по правильному кодингу.
    Отдельное спасибо за обзор моего кода.
    p.s. scasb я пробовал, но там возникла проблема - надо было бы вычислять адрес каждой строки + заносить адрес ее конца (минус нуллбайт) в edi. Это у меня не особо получилось, в итоге я переписал на lodsb. Но раз быстрее, то попробую еще раз со scasb.

    [Ответить]

    dx:

    В твоем коде lodsb меняется на scasb очень просто, я пробовал. Нужно просто поменять esi и edi местами и, собственно, заменить команду.

    [Ответить]

  2. >что конструктором $this->_helper->json? Вероятно, есть уязвимость XSS.
    - он преобразует массивы и строки в json, там не может быть xss.

    >Еще интересно, что будет, если $_GET['query'] будет массивом. Стоит проверить этот момент.
    - будет сгенерировано исключение, страница Application Error вместо JSON ответа, что имхо является вполне корректной работой приложения

    Локализация это правильно, но не во всех случаях. Иногда она совсем-совсем не нужна и только отнимает время.

    Но я таки вынес из ревью полезное замечание - оказывается isset принимает несколько параметров) Спасибо за это

    [Ответить]

  3. Спасибо за обзор.

    [Ответить]


  4. _sheva740 :

    Вот. А моего нету ((((
    А можно узнать какие исходники планируются к просмотру?
    Спасибо.

    [Ответить]

    Kaimi:

    Что осталось и будет прислано, то и планируется

    [Ответить]


  5. Василий :

    Не троллинга ради, но есть несколько замечаний, на которые хотелось бы обратить внимание.

    > Зачем было выделять этот фрагмент в отдельную область видимости, для меня остается загадкой

    Правило хорошего тона — держать область видимости переменной минимально необходимой.

    > особенно учитывая небольшие размеры скрипта и отсутствие существенных объемов данных в памяти, которые освободились бы при выходе из неё.

    Память операционной системе, при выходе из блока, не возвращается, а остается зарезервированной для последующего использования (причем при весьма конкретных обсоятельствах, а не просто так). Слово "освободились" в данном случае неуместно.
    Рекомендуется к прочтению:
    http://assets.en.oreilly.com/1/event/80/Profiling%20memory%20usage%20of%20Perl%20applications%20Presentation.pdf (свежак, очень подробно, маст рид)

    http://www.perlmonks.org/?node_id=803515

    > Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/.

    Вот это откровенно вредный совет. Всегда надо помнить, что $/ — глобальная переменная, со всеми вытекающими. В крупных системах это может привести к очень трудно отлаживаемым ошибкам, а в мелких скриптах использовать эту переменную просто нет смысла, т.к. профита от нее очень мало, а читаемости она не добавляет.

    [Ответить]

    Kaimi:

    1. Приведите пример реального кода из продакшена или из крупных модулей CPAN, где это на 100% выполняется.
    2. Думалось, что Perl в этом смысле схож с неким другим языком, оказалось нет.
    3. И эту глобальную переменную нормальные люди не будут трогать, а если будут, то напишут local и проведут необходимые манипуляции. А если нет, то use constant NEW_LINE => ... было бы таки лучше с моей точки зрения.

    [Ответить]

    Василий:

    > 1. Приведите пример реального кода из продакшена или из крупных модулей CPAN, где это на 100% выполняется.

    Код из продакшена по понятным причинам приводить не буду. По модулям ковыряться просто лениво. В любом случае, это правило вполне очевидно, на мой взгляд.

    > 3. И эту глобальную переменную нормальные люди не будут трогать, а если будут, то напишут local и проведут необходимые манипуляции. А если нет, то use constant NEW_LINE => ... было бы таки лучше с моей точки зрения.

    На аргумент "нормальные люди не будут трогать" в программировании полагаться нельзя. Если вдруг какой–то модуль, который вы используете, вдруг изменит эту переменную, то у вас будут проблемы.
    Если уж очень хочется писать полностью переносимые скрипты, то надо делать что–то типа того, что вы написали: use constant NEW_LINE => $^O =~ /mswin/i ? "\r\n" : "\n" (без упоминания $/).

    [Ответить]

    Kaimi:

    3. Если на этот аргумент не полагаться, то стоит ли в случае с, например, ассемблером изначально полагать, что некий чужой код может вообще перетереть память процесса после старта и мой продукт не заработает? "Что-то типа того" не учитывает особый перенос MacOS.
    Я не против устоявшихся практик, но оверинжинирингом в скриптах на 200 строк тоже заниматься не стоит, тем более негатив был описан на примере крупных вещей, которые пишут множество людей.

  6. >Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/.

    Во-первых, не $/ а $\
    Во-вторых, +1 к вредности этого совета. В крайней случае можно print на say заменить

    [Ответить]

    Kaimi:

    Не $/, а $\? По описанию то все знают, что $/ - input record separator, а $\ - output record separator. Вот только $/ по умолчанию определен в соответствии с принятой в системе секвенцией перевода строки, а $\ - нет.

    say?
    This keyword is available only when the "say" feature is enabled, or when prefixed with CORE:: ; see feature. Alternately, include a use v5.10 or later to the current scope.
    Да ну нафиг, а ещё на целевых системах далеко не всегда свежая версия интерпретатора.

    [Ответить]

    Roman:

    >Не $/, а $\?

    Именно так. Ты же предлагаешь использовать эту переменную вместо "\n" при OUTPUT а не при INPUT :
    "Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/."

    Согласен что по-умолчанию эта $\ не определена, что делает совет еще более странным.
    Если используется perl 5.10 то после use feature 'say'; можно смело писать say 'lalala'.

    [Ответить]

    Kaimi:

    Я её предлагаю именно в виду её определенности нужной последовательностью. Про feature - это то понятно, но к сожалению часто приходится сталкиваться с солярисом, где <=5.8


  7. Daniil Popov :

    Большое спасибо за обзор!
    Особенно за это.

    my $i = 0;
    my %hash_of_counts = map { $i++ => scalar grep {defined && /^\s*\d{20}\s*$/} @{ $_ } } @array_of_rows;

    если кому-нибудь интересно, то вот файлы excel для демонстрации.

    http://ge.tt/2zY07nL?c

    [Ответить]


Оставьте ваш комментарий