Вот и наступило время для первого обзора исходников от Kaimi и dx. Не все исходники попали в этот пост, потому что прислано их было немало. Не огорчайтесь, если не увидели своего творения, мы постараемся включить его в следующие выпуски!
Начнет review Kaimi.
Первый скрипт на Perl прислал Alexandr Alexeev. Сей скрипт показывает уведомления о новых письмах в трее. Ознакомиться с оригинальным кодом можно по этой ссылке. Оформление комментировать здесь и далее не планирую, так как сам предпочитаю так называемый Allman style, но с некоторыми модификациями, да и о вкусах не спорят. Итак, перейдем к авторскому коду:
|
1 2 3 4 5 6 7 8 9 |
{ my @depends = qw/zenity gpg/; my $not_found; for(@depends) { warn "ERROR: $_ not found\n" and ++$not_found if(system("which $_ > /dev/null")); } exit 1 if($not_found); } |
Зачем было выделять этот фрагмент в отдельную область видимости, для меня остается загадкой, особенно учитывая небольшие размеры скрипта и отсутствие существенных объемов данных в памяти, которые освободились бы при выходе из неё. Список зависимостей можно было бы и не выносить в отдельную переменную, а написать, как сделано было в коде ниже:
|
1 2 3 |
for(qw/zenity gpg/) { .... |
Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/.
Смысл использования скобок в предпоследней строке тоже не ясен. Строчка тривиальная, да и далее по коду есть множество случаев опускания скобок в подобных ситуациях. Продолжим.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
my $json; if($opts{'no-master-password'}) { $json = eval { read_file(CONFIG) }; if($@) { die 'Failed to read '.CONFIG."\n"; } } else { my $pw = get_password(); die "get_password() returned undef\n" unless defined $pw; $json = decrypt_file(CONFIG, $pw); unless(defined $json) { message("Invalid password"); exit 1; } } |
Функция decrypt_file используется во всем коде только один раз, CONFIG - глобальная константа. Не знаю, насколько целесообразно было делать генерик функцию и передавать в неё переменную в этом случае.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 |
while(1) { for my $mailbox (@{$conf->{pop3_list}}) { my $pop = new Mail::POP3Client( USER => $mailbox->{user}, PASSWORD => $mailbox->{password}, HOST => $mailbox->{host}, USESSL => ($mailbox->{ssl} != 0), ); my $count = $pop->Count; if($count < 0) { message("$mailbox->{user}: ".$pop->Message); } elsif($count > 0) { message("$mailbox->{user}: $count new message(s)"); if($mailbox->{delete} != 0) { $pop->Delete($_) for (1 .. $count); } } $pop->Close; } sleep $conf->{main}{check_interval}; } |
Объект в цикле создавать не стоит. Корректнее было бы создать его вне цикла, при этом не передавать в конструктор параметры USER и PASSWORD, а в цикле вызывать метод Connect. Хотя я и обещал не комментировать оформление, но все же стоит как-то унифицировать использование скобок и кавычек (в частности, при обращении к элементам ассоциативных массивов: то они есть, то их нет...).
Следующим рассмотрим код, который создает файлы списков для зачисления (Сбербанк, dbf) из таблиц Excel файлов (xls), присланный Даниилом Поповым. Код полностью.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 |
#!/usr/bin/env perl use v5.14; use warnings; use strict; use Encode qw (encode decode); use Spreadsheet::ParseExcel; use Array::Transpose; use List::Util qw (max); use XBase; use locale; use POSIX qw(locale_h); setlocale( LC_CTYPE, "Russian_Russia.866" ); setlocale( LC_COLLATE, 'Russian_Russia.866' ) or die 'locale!'; |
Исходя из кодировки, скрипт используется под win* системами, причем в качестве консольного скрипта, следовательно, указание nix-style пути к интерпретатору не имеет особого смысла. Функция decode, импортированная из модуля Encode, в коде вообще не используется, а прагмы лучше подключать друг рядом с другом (strict, warnings, locale) чисто с эстетической точки зрения.
|
1 2 3 4 5 6 |
my $parser = Spreadsheet::ParseExcel->new(); my $fn = shift @ARGV; my $workbook = $parser->parse("$fn"); unless ( defined $workbook ) { die $parser->error(), ".\n"; } |
Обрамлять переменную в кавычки не стоило. В качестве переноса строки, как я говорил ранее, лучше бы было $/ использовать, да и в Windows \r\n используется по стандарту.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 |
my @array_of_rows; for my $worksheet ( $workbook->worksheets() ) { #Берем информацию о файле # строки my ( $row_min, $row_max ) = $worksheet->row_range(); # колонки my ( $col_min, $col_max ) = $worksheet->col_range(); # перебираем все значения в таблице for my $row ( $row_min .. $row_max ) { my $row_array = []; my $flag = 0; for my $col ( $col_min .. $col_max ) { my $cell = $worksheet->get_cell( $row, $col ); if ( $cell and $cell->value() ) { push $row_array, $cell->value(); ++$flag; } else { push $row_array, undef; } } push @array_of_rows, $row_array if $flag; } } |
В этом фрагменте видим использование push в применении к скаляру. Работать будет, но только для версии Perl >= 5.14 (что и было указано в начале скрипта), но смысл так делать? Подошел бы обычный массив, тем более "The exact behaviour may change in a future version of Perl.".
|
1 2 3 4 5 6 7 8 9 10 |
my $hash_of_counts = {}; for ( my $i = 0 ; $i <= $#array_of_rows ; $i++ ) { $hash_of_counts->{"$i"} = 0; for ( @{ $array_of_rows[$i] } ) { if (defined) { $hash_of_counts->{"$i"}++ if (/^\s*\d{20}\s*$/); } } } |
Этот фрагмент кода дублируется несколько раз, но с разным регулярным выражением. Стоило вынести в отдельную функцию. Переменная в кавычках, и снова зачем-то использован скаляр вместо того, чтобы обычный хэш создать. А ещё этот фрагмент можно заменить чем-нибудь типа:
|
1 2 |
my $i = 0; my %hash_of_counts = map { $i++ => scalar grep {defined && /^\s*\d{20}\s*$/} @{ $_ } } @array_of_rows; |
Комментировать использование модулей Spreadsheet::ParseExcel и XBase не буду, так как не доводилось их использовать.
Хочу сделать общий ко всем исходным кодам комментарий. Определитесь со стилем использования скобочек и кавычек. Выбирайте что-нибудь одно.
|
1 2 3 4 5 6 7 |
$a->{$i} или $a->{"$i"} # Вдруг кому-то нравится брать переменные в кавычки $a->{vasya} или $a->{'vasya'} sleep(1) или sleep 1 # Имеется в виду использование константных строк без переменных внутри "123" или '123' # И стиль написания простых выражений sleep 1 if 0 или if(0) { sleep 1; } |
Напоследок: если ваш код предполагает обработку каких-то специальных входных данных (как последний скрипт), то прикладывайте по возможности пример этих входных данных в виде файла в соответствующем формате.
Продолжает dx.
Перейдем к ассемблеру (MASM32). Morgot B прислал сорс, который ищет файлы с заданными расширениями в указанной директории. Вот он. Начну с самого начала.
|
1 2 3 4 5 |
include \masm32\include\windows.inc include \masm32\include\wininet.inc include \masm32\macros\macros.asm include \masm32\macros\windows.asm uselib kernel32,masm32,user32 |
Макросы - это хорошо. Упрощает код, пишем меньше строк.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
.const ;комментарий автора extnum equ 5 ;может на структуры переделать и lengthof? в общем это к-тво расширений fd WIN32_FIND_DATA <> ;FILE_ATTRIBUTE_DIRECTORY startDir db "E:",0 ;стартовая папка mask0 db "*",0 ;маска поиска buff db 512 dup (0) ; -- вот это, кстати, нигде в программе не используется ;искомые расширения ext1 db "jpg",0 ext2 db "doc",0 ext3 db "docx",0 ext4 db "mp3",0 ext5 db "torrent",0 |
Если это простая программка предназначена исключительно для обучения, не стоит задаваться такими вопросами. В идеале было бы запрашивать расширения для поиска и стартовую директорию у пользователя, а не забивать их в код программы, но здесь это не важно.
|
1 2 3 4 5 6 |
;указатели pext1 dd offset ext1 pext2 dd offset ext2 pext3 dd offset ext3 pext4 dd offset ext4 pext5 dd offset ext5 |
Я так понял, это своеобразный массив указателей на строки, чтобы можно было его перебирать и сравнивать расширение очередного файла с каждым из искомых. В реальности в программе используется только переменная pext1, так не проще ли переписать так:
|
1 2 |
;указатели pext1 dd offset ext1, offset ext2, offset ext3, offset ext4, offset ext5 |
|
1 2 3 4 5 6 |
;счетчики cext1 dd 0 cext2 dd 0 cext3 dd 0 cext4 dd 0 cext5 dd 0 |
Такой же совет могу дать и тут - переписать это как массив dword'ов и его же использовать:
|
1 2 |
;счетчики cext dd dup 5(0) |
Смотрим дальше:
|
1 2 3 4 |
invoke GetProcessHeap mov heapH,eax invoke HeapAlloc,heapH,HEAP_ZERO_MEMORY,1024 mov lpMem,eax |
Нет смысла выделять 1024 байта памяти на куче. Проще и гораздо быстрее будет выделить ее на стеке:
|
1 |
LOCAL mem[1024]: BYTE |
Разумеется, чтобы сделать так, надо сначала образовать стековый фрейм, обернув весь код начиная от метки start и до invoke ExitProcess,0 в какую-то процедуру:
|
1 2 3 4 5 6 7 |
main PROC LOCAL mem[1024]: BYTE ... ret main ENDP |
|
1 |
invoke wsprintf,lpMem,chr$("jpg - %d,doc - %d,docx - %d,mp3 - %d,torrent - %d"),cext1,cext2,cext3,cext4,cext5 |
Если здесь мы перейдем к массиву dword'ов cext, то это выражение запишется так:
|
1 |
invoke wsprintf,lpMem,chr$("jpg - %d,doc - %d,docx - %d,mp3 - %d,torrent - %d"),[cext],[cext + 4],[cext + 8],[cext + 12],[cext + 16] |
А отсюда уже легко перейти к циклу по всем переменным из массива, затолкнув их в стек, а потом вызвав wsprintf (хотя это необязательно, так как набор и количество искомых расширений фиксированы). Идем дальше:
|
1 2 |
push offset startDir call findAll |
Тут можно было использовать invoke, тем более, прототип для функции findAll написан.
|
1 2 3 4 5 |
invoke lstrcat,addr buf,offset mask0 ;добавляем маску invoke lstrlen,p1 ;вычисляем длину папки с маской mov esi,eax add esi,sizeof mask0 ;добавляем длину маски mov byte ptr buf[esi],0 ;добавляем нуллбайт |
Зачем все это? Функция WinAPI lstrcat всегда дописывает нуллбайт в конец строки, поэтому все действия с ручным его дописыванием лишние.
|
1 |
invoke FindFirstFile,addr buf,offset fd |
Кстати, не понял, почему структура WIN32_FIND_DATA (fd) выделена в куче (точнее, в секции данных). Это убило часть возможностей функции findAll, и она никогда не сможет работать многопоточно. Впрочем, счетчики найденных расширений находятся там же, поэтому это скорее не недостаток, а упрощение.
|
1 2 |
print "some error with FindFirstFile" ;уведомляем и выходим ret |
Я бы вывел ошибку в MessageBox, хотя это не столь важно, так как программа обучающая.
Далее я вижу практически полное дублирование кода, что, естественно, не есть хорошо. Следовало бы реорганизовать код или вынести повторяющиеся моменты в отдельную функцию.
|
1 2 3 4 5 |
invoke lstrlen,offset fd.cFileName mov len1,eax push len1 ;длина имени файла передаем в функу push offset fd.cFileName ;адрес имени файла call GetExt ;поиск валидного расширения |
Опять-таки, можно было бы заменить вызов через push-call на invoke. Кроме того, переменная len1, по сути, не нужна - можно было сделать push eax сразу после вызова lstrlen. Да и, чего уж там, по-хорошему в функцию GetExt надо было передать просто строку, а длину она бы посчитала сама, findAll не обязана за нее это делать. Длина потребовалась, если бы GetExt принимала какие-то двоичные данные - для них просчитать длину невозможно.
Перейдем теперь к самой функции GetExt:
|
1 2 3 4 5 6 7 8 |
std ;ищет расширение файла c конца parse_ext: ;search extension lodsb cmp al,'.' je get_ext loop parse_ext cld |
Этот кусок можно было бы несколько ускорить и упростить, использовав команду scasb вместо lodsb и явного сравнения (не забыв учесть, что scasb работает с регистром edi, а не esi).
Пожалуй, это все, что я хотел сказать по данному исходному коду.
Вердикт: код не очень хороший (это простительно, так как автор, видимо, только учится), его можно сильно улучшить, есть, чему еще учиться. Из явных грехов видно несколько разных видов выделения памяти, хотя можно было бы все выделять на стеке (в данном случае, так как мы не выделяем больших объемов памяти), сделав тем самым все функции потокобезопасными; попеременное использование то макросов MASM32 вроде .if - .else, то операций вроде cmp - je (т.е. опять-таки нет единообразия, код тяжелее читается). Разные функции написаны с разными "конвенциями" вызовов - GetExt сохраняет регистры, findAll - нет. Я бы посоветовал использовать везде одно и то же общепринятое, например, stdcall: сохранять регистры esi, edi и ebx, значения передавать через стек в обратном порядке и очищать стек внутри функции, результат возвращать через eax.
Перейдем к рассмотрению следующего исходного кода. Это "контроллер в некотором Zend Framework приложении для страницы управления проектами". Так как с Zend framework'ом мне приходилось работать очень давно и в течение достаточно короткого промежутка времени, то в плане архитектуры и использования методов Zend'а никаких рекомендаций дать я не смогу. Поэтому просто посмотрим на недостатки в самом коде. Вот он полностью.
Во-первых, мне непонятно, почему прямо в коде присутствуют строки на русском языке. Наверняка Zend поддерживает локализацию. Хотя, вероятно, проект не планируется переводить на другие языки, он всегда будет поддерживаться только на русском, поэтому это нельзя считать серьезным недостатком, но строки вынести в отдельный ресурс я бы все-таки посоветовал - было бы гораздо проще править орфографические и пунктуационные ошибки без изменения самого кода, как минимум.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 |
$data = Zend_Db_Table_Abstract::getDefaultAdapter() ->fetchAll('SELECT email FROM ' . TABLE_PREF . 'users ' .'WHERE email LIKE ?' .'AND role=1 ' //!! Роль номер 1 - клиенты .'LIMIT 20;' , // Не больше 20 в подсказке '%' . ($_GET['query']) . '%'); $suggestions = array(); foreach($data as $email) $suggestions[] = $email['email']; return $this->_helper->json(array( 'query' => $_GET['query'], 'suggestions' => $suggestions )); |
По этому куску кода не могу в целом ничего сказать, но немного смущает полное отсутствие проверок входящих значений и экранирования. Метод fetchAll, скорее всего, производит экранирование, но что с методом/конструктором $this->_helper->json? Вероятно, есть уязвимость XSS. Еще интересно, что будет, если $_GET['query'] будет массивом. Стоит проверить этот момент.
|
1 2 3 |
// Запрос на создание нового заказа if(isset($_POST['orderEmail']) && isset($_POST['project']) && $_POST['orderEmail'] && $_POST['project']) |
Фукнция isset умеет принимать несколько аргументов сразу. Этот фрагмент кода можно было написать так:
|
1 2 3 |
// Запрос на создание нового заказа if(isset($_POST['orderEmail'], $_POST['project']) && $_POST['orderEmail'] && $_POST['project']) |
Еще я бы вместо явных проверок $_POST['orderEmail'] и $_POST['project'] использовал бы функцию empty.
|
1 2 3 |
if(isset($_POST['deleting']) && $_POST['deleting'] && isset($_POST['deleting']) |
Странное дублирование.
Теперь итоги. В целом код выглядит весьма неплохо. В некоторых местах отсутствуют проверки ошибок (строки 122 и 129, например). Не исключаю вариант, что они там и не нужны. Отсутствует поддержка локализации, об этом я уже говорил. Вроде бы в критическим местах даже есть защита от CSRF, но не могу точно сказать, будет ли она хорошо работать. Больше, пожалуй, добавить ничего не могу. Вызываются методы неизвестного мне фреймворка, поэтому не могу полностью оценить, насколько это эффективно и безопасно.
Вот и всё на сегодня. Присылайте новые исходники, и мы обязательно постараемся их рассмотреть. До встречи в следующем Code Review!
Тема получилась очень интересная и полезная. Надеюсь, будет еще продолжение, т.к. реально не у кого часто спросить по качеству кода, или почитать советы по правильному кодингу.
Отдельное спасибо за обзор моего кода.
p.s. scasb я пробовал, но там возникла проблема - надо было бы вычислять адрес каждой строки + заносить адрес ее конца (минус нуллбайт) в edi. Это у меня не особо получилось, в итоге я переписал на lodsb. Но раз быстрее, то попробую еще раз со scasb.
В твоем коде lodsb меняется на scasb очень просто, я пробовал. Нужно просто поменять esi и edi местами и, собственно, заменить команду.
>что конструктором $this->_helper->json? Вероятно, есть уязвимость XSS.
- он преобразует массивы и строки в json, там не может быть xss.
>Еще интересно, что будет, если $_GET['query'] будет массивом. Стоит проверить этот момент.
- будет сгенерировано исключение, страница Application Error вместо JSON ответа, что имхо является вполне корректной работой приложения
Локализация это правильно, но не во всех случаях. Иногда она совсем-совсем не нужна и только отнимает время.
Но я таки вынес из ревью полезное замечание - оказывается isset принимает несколько параметров) Спасибо за это
Спасибо за обзор.
Вот. А моего нету ((((
А можно узнать какие исходники планируются к просмотру?
Спасибо.
Что осталось и будет прислано, то и планируется
Не троллинга ради, но есть несколько замечаний, на которые хотелось бы обратить внимание.
> Зачем было выделять этот фрагмент в отдельную область видимости, для меня остается загадкой
Правило хорошего тона — держать область видимости переменной минимально необходимой.
> особенно учитывая небольшие размеры скрипта и отсутствие существенных объемов данных в памяти, которые освободились бы при выходе из неё.
Память операционной системе, при выходе из блока, не возвращается, а остается зарезервированной для последующего использования (причем при весьма конкретных обсоятельствах, а не просто так). Слово "освободились" в данном случае неуместно.
Рекомендуется к прочтению:
http://assets.en.oreilly.com/1/event/80/Profiling%20memory%20usage%20of%20Perl%20applications%20Presentation.pdf (свежак, очень подробно, маст рид)
http://www.perlmonks.org/?node_id=803515
> Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/.
Вот это откровенно вредный совет. Всегда надо помнить, что $/ — глобальная переменная, со всеми вытекающими. В крупных системах это может привести к очень трудно отлаживаемым ошибкам, а в мелких скриптах использовать эту переменную просто нет смысла, т.к. профита от нее очень мало, а читаемости она не добавляет.
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" (без упоминания $/).
3. Если на этот аргумент не полагаться, то стоит ли в случае с, например, ассемблером изначально полагать, что некий чужой код может вообще перетереть память процесса после старта и мой продукт не заработает? "Что-то типа того" не учитывает особый перенос MacOS.
Я не против устоявшихся практик, но оверинжинирингом в скриптах на 200 строк тоже заниматься не стоит, тем более негатив был описан на примере крупных вещей, которые пишут множество людей.
>Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/.
Во-первых, не $/ а $\
Во-вторых, +1 к вредности этого совета. В крайней случае можно print на say заменить
Не $/, а $\? По описанию то все знают, что $/ - 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.
Да ну нафиг, а ещё на целевых системах далеко не всегда свежая версия интерпретатора.
>Не $/, а $\?
Именно так. Ты же предлагаешь использовать эту переменную вместо "\n" при OUTPUT а не при INPUT :
"Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/."
Согласен что по-умолчанию эта $\ не определена, что делает совет еще более странным.
Если используется perl 5.10 то после use feature 'say'; можно смело писать say 'lalala'.
Я её предлагаю именно в виду её определенности нужной последовательностью. Про feature - это то понятно, но к сожалению часто приходится сталкиваться с солярисом, где <=5.8
Большое спасибо за обзор!
Особенно за это.
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