Code review #1 Вот и наступило время для первого обзора исходников от Kaimi и dx. Не все исходники попали в этот пост, потому что прислано их было немало. Не огорчайтесь, если не увидели своего творения, мы постараемся включить его в следующие выпуски! Начнет review Kaimi. Первый скрипт на Perl прислал Alexandr Alexeev. Сей скрипт показывает уведомления о новых письмах в трее. Ознакомиться с оригинальным кодом можно по этой ссылке. Оформление комментировать здесь и далее не планирую, так как сам предпочитаю так называемый Allman style, но с некоторыми модификациями, да и о вкусах не спорят. Итак, перейдем к авторскому коду: PHP: { 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); } Зачем было выделять этот фрагмент в отдельную область видимости, для меня остается загадкой, особенно учитывая небольшие размеры скрипта и отсутствие существенных объемов данных в памяти, которые освободились бы при выходе из неё. Список зависимостей можно было бы и не выносить в отдельную переменную, а написать, как сделано было в коде ниже: PHP: for(qw/zenity gpg/) { .... Для вывода информационных сообщений вместо вбивания статичного символа переноса строки лучше все же использовать специальную переменную $/. Смысл использования скобок в предпоследней строке тоже не ясен. Строчка тривиальная, да и далее по коду есть множество случаев опускания скобок в подобных ситуациях. Продолжим. PHP: 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 - глобальная константа. Не знаю, насколько целесообразно было делать генерик функцию и передавать в неё переменную в этом случае. PHP: 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), присланный Даниилом Поповым. Код полностью. PHP: #!/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) чисто с эстетической точки зрения. PHP: my $parser = Spreadsheet::ParseExcel->new(); my $fn = shift @ARGV; my $workbook = $parser->parse("$fn"); unless ( defined $workbook ) { die $parser->error(), ".\n"; } Обрамлять переменную в кавычки не стоило. В качестве переноса строки, как я говорил ранее, лучше бы было $/ использовать, да и в Windows \r\n используется по стандарту. PHP: 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.". PHP: 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*$/); } } } Этот фрагмент кода дублируется несколько раз, но с разным регулярным выражением. Стоило вынести в отдельную функцию. Переменная в кавычках, и снова зачем-то использован скаляр вместо того, чтобы обычный хэш создать. А ещё этот фрагмент можно заменить чем-нибудь типа: PHP: my $i = 0; my %hash_of_counts = map { $i++ => scalar grep {defined && /^\s*\d{20}\s*$/} @{ $_ } } @array_of_rows; Комментировать использование модулей Spreadsheet:arseExcel и XBase не буду, так как не доводилось их использовать. Хочу сделать общий ко всем исходным кодам комментарий. Определитесь со стилем использования скобочек и кавычек. Выбирайте что-нибудь одно. PHP: $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 прислал сорс, который ищет файлы с заданными расширениями в указанной директории. Вот он. Начну с самого начала. Перейдем к ассемблеру (MASM32). Morgot B прислал сорс, который ищет файлы с заданными расширениями в указанной директории. Вот он. Начну с самого начала. PHP: include \masm32\include\windows.inc include \masm32\include\wininet.inc include \masm32\macros\macros.asm include \masm32\macros\windows.asm uselib kernel32,masm32,user32 Макросы - это хорошо. Упрощает код, пишем меньше строк. PHP: .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 Если это простая программка предназначена исключительно для обучения, не стоит задаваться такими вопросами. В идеале было бы запрашивать расширения для поиска и стартовую директорию у пользователя, а не забивать их в код программы, но здесь это не важно. PHP: ;указатели pext1 dd offset ext1 pext2 dd offset ext2 pext3 dd offset ext3 pext4 dd offset ext4 pext5 dd offset ext5 Я так понял, это своеобразный массив указателей на строки, чтобы можно было его перебирать и сравнивать расширение очередного файла с каждым из искомых. В реальности в программе используется только переменная pext1, так не проще ли переписать так: PHP: ;указатели pext1 dd offset ext1, offset ext2, offset ext3, offset ext4, offset ext5 PHP: ;счетчики cext1 dd 0 cext2 dd 0 cext3 dd 0 cext4 dd 0 cext5 dd 0 Такой же совет могу дать и тут - переписать это как массив dword'ов и его же использовать: PHP: ;счетчики cext dd dup 5(0) Смотрим дальше: PHP: invoke GetProcessHeap mov heapH,eax invoke HeapAlloc,heapH,HEAP_ZERO_MEMORY,1024 mov lpMem,eax Нет смысла выделять 1024 байта памяти на куче. Проще и гораздо быстрее будет выделить ее на стеке: PHP: LOCAL mem[1024]: BYTE Разумеется, чтобы сделать так, надо сначала образовать стековый фрейм, обернув весь код начиная от метки start и до invoke ExitProcess,0 в какую-то процедуру: PHP: main PROC LOCAL mem[1024]: BYTE ... ret main ENDP PHP: invoke wsprintf,lpMem,chr$("jpg - %d,doc - %d,docx - %d,mp3 - %d,torrent - %d"),cext1,cext2,cext3,cext4,cext5 Если здесь мы перейдем к массиву dword'ов cext, то это выражение запишется так: PHP: invoke wsprintf,lpMem,chr$("jpg - %d,doc - %d,docx - %d,mp3 - %d,torrent - %d"),[cext],[cext + 4],[cext + 8],[cext + 12],[cext + 16] А отсюда уже легко перейти к циклу по всем переменным из массива, затолкнув их в стек, а потом вызвав wsprintf (хотя это необязательно, так как набор и количество искомых расширений фиксированы). Идем дальше: PHP: push offset startDir call findAll Тут можно было использовать invoke, тем более, прототип для функции findAll написан. PHP: invoke lstrcat,addr buf,offset mask0 ;добавляем маску invoke lstrlen,p1 ;вычисляем длину папки с маской mov esi,eax add esi,sizeof mask0 ;добавляем длину маски mov byte ptr buf[esi],0 ;добавляем нуллбайт Зачем все это? Функция WinAPI lstrcat всегда дописывает нуллбайт в конец строки, поэтому все действия с ручным его дописыванием лишние. PHP: invoke FindFirstFile,addr buf,offset fd Кстати, не понял, почему структура WIN32_FIND_DATA (fd) выделена в куче (точнее, в секции данных). Это убило часть возможностей функции findAll, и она никогда не сможет работать многопоточно. Впрочем, счетчики найденных расширений находятся там же, поэтому это скорее не недостаток, а упрощение. PHP: print "some error with FindFirstFile" ;уведомляем и выходим ret Я бы вывел ошибку в MessageBox, хотя это не столь важно, так как программа обучающая. Далее я вижу практически полное дублирование кода, что, естественно, не есть хорошо. Следовало бы реорганизовать код или вынести повторяющиеся моменты в отдельную функцию. PHP: 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: PHP: 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 поддерживает локализацию. Хотя, вероятно, проект не планируется переводить на другие языки, он всегда будет поддерживаться только на русском, поэтому это нельзя считать серьезным недостатком, но строки вынести в отдельный ресурс я бы все-таки посоветовал - было бы гораздо проще править орфографические и пунктуационные ошибки без изменения самого кода, как минимум. PHP: $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'] будет массивом. Стоит проверить этот момент. PHP: // Запрос на создание нового заказа if(isset($_POST['orderEmail']) && isset($_POST['project']) && $_POST['orderEmail'] && $_POST['project']) Фукнция isset умеет принимать несколько аргументов сразу. Этот фрагмент кода можно было написать так: PHP: // Запрос на создание нового заказа if(isset($_POST['orderEmail'], $_POST['project']) && $_POST['orderEmail'] && $_POST['project']) Еще я бы вместо явных проверок $_POST['orderEmail'] и $_POST['project'] использовал бы функцию empty. PHP: if(isset($_POST['deleting']) && $_POST['deleting'] && isset($_POST['deleting']) Странное дублирование. Теперь итоги. В целом код выглядит весьма неплохо. В некоторых местах отсутствуют проверки ошибок (строки 122 и 129, например). Не исключаю вариант, что они там и не нужны. Отсутствует поддержка локализации, об этом я уже говорил. Вроде бы в критическим местах даже есть защита от CSRF, но не могу точно сказать, будет ли она хорошо работать. Больше, пожалуй, добавить ничего не могу. Вызываются методы неизвестного мне фреймворка, поэтому не могу полностью оценить, насколько это эффективно и безопасно. Вот и всё на сегодня. Присылайте новые исходники, и мы обязательно постараемся их рассмотреть. До встречи в следующем Code Review! Воскресенье, 5. Август 2012 dx-kaimi http://kaimi.ru/2012/08/code-review-1/
