Code review #1

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

Начнет review Kaimi.

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

{my@depends=qw/zenity gpg/;my$not_found;for(@depends){warn"ERROR: $_ not found\n"and++$not_foundif(system("which $_ > /dev/null"));}exit1if($not_found);}

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

for(qw/zenity gpg/){....

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

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"unlessdefined$pw;$json= decrypt_file(CONFIG,$pw);unless(defined$json){
    message("Invalid password");exit1;}}

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

while(1){formy$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), присланный Даниилом Поповым. Код полностью.

#!/usr/bin/env perluse v5.14;use warnings;use strict;use Encode qw(encode decode);use Spreadsheet::ParseExcel;use Array::Transpose;use List::Utilqw(max);use XBase;use locale;use POSIX qw(locale_h);
setlocale( LC_CTYPE,"Russian_Russia.866");
setlocale( LC_COLLATE,'Russian_Russia.866')ordie'locale!';

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

my$parser= Spreadsheet::ParseExcel->new();my$fn=shift@ARGV;my$workbook=$parser->parse("$fn");unless(defined$workbook){die$parser->error(),".\n";}

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

my@array_of_rows;formy$worksheet($workbook->worksheets()){
 
    #Берем  информацию о файле# строкиmy($row_min,$row_max)=$worksheet->row_range();
 
    # колонкиmy($col_min,$col_max)=$worksheet->col_range();
 
    # перебираем все значения в таблицеformy$row($row_min..$row_max){my$row_array=[];my$flag=0;formy$col($col_min..$col_max){my$cell=$worksheet->get_cell($row,$col);if($celland$cell->value()){push$row_array,$cell->value();++$flag;}else{push$row_array,undef;}}push@array_of_rows,$row_arrayif$flag;}}

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

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*$/);}}}

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

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

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

$a->{$i} или $a->{"$i"}# Вдруг кому-то нравится брать переменные в кавычки$a->{vasya} или $a->{'vasya'}sleep(1) или sleep1# Имеется в виду использование константных строк без переменных внутри"123" или '123'# И стиль написания простых выраженийsleep1if0 или if(0){sleep1;}

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

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

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

include \masm32\include\windows.inc
include \masm32\include\wininet.inc
include \masm32\macros\macros.asm
include \masm32\macros\windows.asm
uselib kernel32,masm32,user32

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

.const
;комментарий автора
extnum equ5;может на структуры переделать и lengthof? в общем это к-тво расширений
 
fd WIN32_FIND_DATA <> ;FILE_ATTRIBUTE_DIRECTORY
startDir db"E:",0;стартовая папка
mask0 db"*",0;маска поиска
buff db512 dup (0); -- вот это, кстати, нигде в программе не используется
 
;искомые расширения
ext1 db"jpg",0
ext2 db"doc",0
ext3 db"docx",0
ext4 db"mp3",0
ext5 db"torrent",0

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

;указатели
pext1 dd offset ext1
pext2 dd offset ext2
pext3 dd offset ext3
pext4 dd offset ext4
pext5 dd offset ext5

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

;указатели
pext1 dd offset ext1, offset ext2, offset ext3, offset ext4, offset ext5

;счетчики
cext1 dd0
cext2 dd0
cext3 dd0
cext4 dd0
cext5 dd0

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

;счетчики
cext dd dup 5(0)

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

invoke GetProcessHeap
	mov heapH,eax
invoke HeapAlloc,heapH,HEAP_ZERO_MEMORY,1024mov lpMem,eax

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

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

main PROC
LOCAL mem[1024]:BYTE
 
...
 
ret
main ENDP

invoke wsprintf,lpMem,chr$("jpg - %d,doc - %d,docx - %d,mp3 - %d,torrent - %d"),cext1,cext2,cext3,cext4,cext5

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

invoke wsprintf,lpMem,chr$("jpg - %d,doc - %d,docx - %d,mp3 - %d,torrent - %d"),[cext],[cext +4],[cext +8],[cext +12],[cext +16]

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

push offset startDir
call findAll

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

invoke lstrcat,addr buf,offset mask0 ;добавляем маску
invoke lstrlen,p1 ;вычисляем длину папки с маскойmovesi,eaxaddesi,sizeof mask0 ;добавляем длину маскиmovbyte ptr buf[esi],0;добавляем нуллбайт

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

invoke FindFirstFile,addr buf,offset fd

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

	print "some error with FindFirstFile";уведомляем и выходимret

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

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

	invoke lstrlen,offset fd.cFileName
	mov len1,eaxpush len1 ;длина имени файла передаем в функуpush offset fd.cFileName ;адрес имени файлаcall GetExt ;поиск валидного расширения

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

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

std;ищет расширение файла c конца
 
parse_ext:;search extensionlodsbcmpal,'.'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 поддерживает локализацию. Хотя, вероятно, проект не планируется переводить на другие языки, он всегда будет поддерживаться только на русском, поэтому это нельзя считать серьезным недостатком, но строки вынести в отдельный ресурс я бы все-таки посоветовал - было бы гораздо проще править орфографические и пунктуационные ошибки без изменения самого кода, как минимум.

$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($dataas$email)$suggestions[]=$email['email'];
 
            return$this->_helper->json(array('query'=>$_GET['query'],'suggestions'=>$suggestions));

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

// Запрос на создание нового заказаif(isset($_POST['orderEmail'])&&isset($_POST['project'])&&$_POST['orderEmail']&&$_POST['project'])

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

// Запрос на создание нового заказаif(isset($_POST['orderEmail'],$_POST['project'])&&$_POST['orderEmail']&&$_POST['project'])

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

if(isset($_POST['deleting'])&&$_POST['deleting']&&isset($_POST['deleting'])

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

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

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

Также рекомендую почитать

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

• Комментарии