ревью кода
От: Hydrophobia  
Дата: 04.05.12 05:51
Оценка:
Доброе время суток.

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

задание: реализовать алгоритм сортировки строк в файле за минимальное время.

zip-архив здесь: http://files.rsdn.ru/99432/file_sort.zip

От Вас необходимы замечания и конструктивная критика кода.

Заранее спасибо.


21.06.12 15:43: Перенесено модератором из 'WIN API' — Hacker_Delphi
Re: ревью кода
От: acDev Россия  
Дата: 04.05.12 06:34
Оценка:
На беглый взгляд — очень адекватный кодес.
Может работодатели просто не способны понять как там всё работает. Или же просто goto "испугались"))
Re: ревью кода
От: elmal  
Дата: 04.05.12 07:18
Оценка: +1
Здравствуйте, Hydrophobia, Вы писали:

H>От Вас необходимы замечания и конструктивная критика кода.

Из хорошего — отформатировано нормально. Одно место увидел только, где лишняя строка.
Из плохого:
1) Методы длинные, читается хреново. for внутри if который внутри while, и это несколько раз в одном методе;
2) Копипаст. Пример копипаста:

if(i>=lines0.size()){
if(!buff0->eof()){
buff0->read(lines0);
i=0;
}
else{
flush1=true;
break;
}
}

if(j>=lines1.size()){
if(!buff1->eof()){
buff1->read(lines1);
j=0;
}
else{
flush0=true;
break;
}
}

Есть более мелкий копипаст, вроде nodes[i].range_.first — ну вот можно было nodes[i].range_ в отдельную переменную выделить ? Ведь задолбаешься как сам набирать, так и читать это все.
3) Явное управление памятью — вызов delete. Не знаю, может сейчас явно дергать delete стало хорошим тоном, но раньше вроде как за это по рукам били;
4) Обработка ошибок через коды возврата. Я конечно понимаю, что если пользоваться delete явно, то исключения использовать нельзя, иначе схлопочешь утечку. Но вот если переходить на использование памяти со счетчиками ссылок и пользоваться всякими смарт указателями, и от всяких delete избавиться, то и необходимость в кодах возврата не будет;
5) Смотрел не очень сильно, но мне кажется, что код самой сортировки можно отделить от чтения файла все таки получше;
6) goto тоже как то смотрится не ахти. Ну используешь ресурсы, которые нужно закрывать — ну оберни их в какой нидь автозакрыватель или еще что. Ведь ошибиться с забытием закрыть файл элементарно, а тут еще и goto в не самом коротком методе. Как это потом поддерживать?
7) Полное отсутствие комментариев тоже не говорит в пользу кода (хотя — вроде один насчитал. Только не в том месте, именно внутри методов комментарии как раз и не нужны обычно).
8) Хватит пока . Многопоточность тоже не очень понравилась, но тоже больше за счет неструктурированности и размазанности. Будет ли это работать нормально и нет ли там багов — даже не смотрел, ибо читается хреново, и просторов для улучшения миллион.

Короче — на джуниора тянешь, у джуниоров я и не такое видел. Отформатировано — и то большой плюс, обычно и не форматируют ни черта. Но до кода конкретного пацана сильно не дотягиваешь.
Re: ревью кода
От: okman Беларусь https://searchinform.ru/
Дата: 04.05.12 07:58
Оценка: 1 (1) +1
Здравствуйте, Hydrophobia, Вы писали:

H>задание: реализовать алгоритм сортировки строк в файле за минимальное время.

H>zip-архив здесь: http://files.rsdn.ru/99432/file_sort.zip
H>От Вас необходимы замечания и конструктивная критика кода.

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

Сначала общие замечания.

Первое, что бросается в глаза — это то, что код непонятно то ли C-шный, то ли плюсовый.
Но то, что он не использует на всю мощность возможности C++, это точно.
Например, RAII почти не видно. Вот в этом фрагменте (fsort.cpp, 242):
  while(true){
    EnterCriticalSection(&arg->crit_section_);
    if(save_result){
      arg->files_.push(out_fname);
      out_fname=L"";
      InterlockedDecrement(&arg->workers_);
      save_result=false;
    }

подозрение, что если arg->files_.push кинет исключение, LeaveCriticalSection никогда не
будет вызвана и возникнет дедлок. Там еще по коду голые new/delete, ручной close() и т.п.
Все пересмотреть и запаковать в RAII-обертки.
Ну и передача структур аргументами функций, а-ля Win32 API — как-то не очень.

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

Вот. Это, пожалуй, основное. Из плюсов отмечу, что код относительно легко читается, да и
схема именования вполне себе. А еще мне понравилось, что везде проверяются коды возвратов и
всякие ошибочные ситуации, политика исключений тоже отчетливо выражена (nothrow).
Очень многие на этом пытаются экономить. За это все — жирный плюс.

Теперь всякие зацепки.

1. При компиляции данного кода вываливается несколько предупреждений (на /W4).
Рекомендую поправить, особенно "С4018 — signed/unsigned mismatch".

2. Я заметил, что типы-размеры у Вас везде unsigned int. Почему ?
Если работаете с памятью и особенно файлами, нельзя исключать ситуации с превышением 4GB.
Тут даже не size_t нужен (так как он будет эквивалентен unsigned int на x86), а что-то типа bigsize_t.
Кстати, компиляция под x64 подтверждает: "C4267 — conversion from <...> to <...>, possible loss of data".

3. В Index.cpp (line 42) есть такой фрагмент:
GlobalMemoryStatusEx(&statex);
unsigned int avail_mem=(statex.ullTotalPhys>>32)?unsigned int(-1):(unsigned int)statex.ullAvailPhys;

По-моему, так вычислять доступную память неправильно. Например, если 32-битная версия приложения
будет запущена на 64-битной Windows, она покажет всю доступную оперативку, хотя реально будет
доступно не более 2 гигов.

4. По коду, чисто субъективно. Вместо static я бы использовал anonymous namespaces, а из
fsort.h поубирал бы все, не имеющее прямого отношения к API. И макросы тоже бы выкинул, по возможности, в
том числе и те, которые используются для кодов ошибок и неймспейсов. Макросы — зло, почти всегда.

5. Ну и совсем IMHO — стиль оформления очень уж скомпрессированный.
Надо где-то и пробел вставлять иногда, и строку пустую.
Вот это, к примеру:
std::stack<block_t*> blocks;
  for(std::size_t i=0;i<nodes.size();++i){
    block_t*block=new(std::nothrow) block_t;
    if(block){
      block->reserve(nodes[i].range_.second-nodes[i].range_.first+1);
      for(unsigned int j=nodes[i].range_.first;j<=nodes[i].range_.second;++j)
        block->push_back(lines[j]);
      blocks.push(block);
    }
  }

приятнее читается в таком обличии:
std::stack<block_t*> blocks;

for(std::size_t i=0; i < nodes.size(); ++i)
{
    block_t * block = new(std::nothrow) block_t;

    if(block)
    {
        block->reserve(nodes[i].range_.second-nodes[i].range_.first + 1);
        for(unsigned int j = nodes[i].range_.first;j <= nodes[i].range_.second; ++j)
        block->push_back(lines[j]);
        blocks.push(block);
    }
}


Короче, если кое-где шлифануть, получится на добрую четверку.
Re: ревью кода
От: DarkTranquillity  
Дата: 04.05.12 08:18
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

H>Доброе время суток.


H>Устал получать отказы от работодателей после выполения тестовых заданий, поэтому расчитываю на общественное мнение.


H>задание: реализовать алгоритм сортировки строк в файле за минимальное время.


H>zip-архив здесь: http://files.rsdn.ru/99432/file_sort.zip


H>От Вас необходимы замечания и конструктивная критика кода.


H>Заранее спасибо.


После беглого просмотра кода и комментариев выше почувствовал себя лохом.
Единственное, что сразу бросается в глаза — это форматирование (принято в русском после знаков препинания и т.п. ставить пробелы), ну и как уже заметили, кое-где опущено почему-то RAII.
В алгоритм не вникал, не понял почему он такой сложный. За распараллеливанием следите сами? Ну и число потоков естественно нет смысла делать больше числа процессоров.
Re[2]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 08:43
Оценка: -1
Здравствуйте, elmal, Вы писали:

E>Из плохого:

E>1) Методы длинные, читается хреново. for внутри if который внутри while, и это несколько раз в одном методе;

разбил на мелкие функции с одной обязаностью/стратегией.

E>2) Копипаст. Пример копипаста:


убрал.

E>Есть более мелкий копипаст, вроде nodes[i].range_.first — ну вот можно было nodes[i].range_ в отдельную переменную выделить ? Ведь задолбаешься как сам набирать, так и читать это все.


добавил временные переменные для range — код стал читабельней.

E>3) Явное управление памятью — вызов delete. Не знаю, может сейчас явно дергать delete стало хорошим тоном, но раньше вроде как за это по рукам били;


GC нет, разве что на объекты перейти.

E>4) Обработка ошибок через коды возврата. Я конечно понимаю, что если пользоваться delete явно, то исключения использовать нельзя, иначе схлопочешь утечку. Но вот если переходить на использование памяти со счетчиками ссылок и пользоваться всякими смарт указателями, и от всяких delete избавиться, то и необходимость в кодах возврата не будет;


согласен — надо осваивать boost.

E>5) Смотрел не очень сильно, но мне кажется, что код самой сортировки можно отделить от чтения файла все таки получше;


да, отделяется легко, вероятно лимит времени на тестовое задание породил такую кашу либо я был слеп

E>6) goto тоже как то смотрится не ахти. Ну используешь ресурсы, которые нужно закрывать — ну оберни их в какой нидь автозакрыватель или еще что. Ведь ошибиться с забытием закрыть файл элементарно, а тут еще и goto в не самом коротком методе. Как это потом поддерживать?


да, надо все таки на объекты переходить.

E>7) Полное отсутствие комментариев тоже не говорит в пользу кода (хотя — вроде один насчитал. Только не в том месте, именно внутри методов комментарии как раз и не нужны обычно).

E>8) Хватит пока . Многопоточность тоже не очень понравилась, но тоже больше за счет неструктурированности и размазанности. Будет ли это работать нормально и нет ли там багов — даже не смотрел, ибо читается хреново, и просторов для улучшения миллион.

E>Короче — на джуниора тянешь, у джуниоров я и не такое видел. Отформатировано — и то большой плюс, обычно и не форматируют ни черта. Но до кода конкретного пацана сильно не дотягиваешь.


видимо пора перечитать C++ In Depth, Мартина, Мейерса и GoF.

Большое спасибо за подробное ревью кода!
Re[2]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 08:49
Оценка:
Здравствуйте, DarkTranquillity, Вы писали:

DT>Единственное, что сразу бросается в глаза — это форматирование (принято в русском после знаков препинания и т.п. ставить пробелы), ну и как уже заметили, кое-где опущено почему-то RAII.


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

DT>В алгоритм не вникал, не понял почему он такой сложный. За распараллеливанием следите сами? Ну и число потоков естественно нет смысла делать больше числа процессоров.


вдохновлялся с coreutils::sort : файл бьется на сегменты заданного размера, каждый сегмент бьется на N частей и сортируется std::sort, после чего N сегметов дампятся во временный файл с использованием сортировки слиянием. В конце набор файлов попарно мержится сортировкой слиянием в новый файл, последний файл переименовывается в целевой.

Спасибо за ревью!
Re[2]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 09:05
Оценка:
Здравствуйте, okman, Вы писали:

O>Первое, что бросается в глаза — это то, что код непонятно то ли C-шный, то ли плюсовый.

O>Но то, что он не использует на всю мощность возможности C++, это точно.
O>Например, RAII почти не видно. Вот в этом фрагменте (fsort.cpp, 242):

O>подозрение, что если arg->files_.push кинет исключение, LeaveCriticalSection никогда не

O>будет вызвана и возникнет дедлок. Там еще по коду голые new/delete, ручной close() и т.п.
O>Все пересмотреть и запаковать в RAII-обертки.

да, все таки надо переходить на объекты.

O>Ну и передача структур аргументами функций, а-ля Win32 API — как-то не очень.


а как правильно?

O>И где юнит-тесты ? Это же плюс сто баллов на любом собеседовании.

O>Хорошо, если бы сразу после компиляции можно было запустить прогон тестов с
O>выкладками по перфомансу и тестовым файлом для сортировки. Лично в моих глазах этот
O>код сразу бы выиграл. А так — надо самому текстовый файл накатывать, разбираться как
O>этот API работает (из тестов сразу бы было видно, даже документации не нужно).

хорошее замечание, буду изучать эту тему.


O>1. При компиляции данного кода вываливается несколько предупреждений (на /W4).

O>Рекомендую поправить, особенно "С4018 — signed/unsigned mismatch".


O>2. Я заметил, что типы-размеры у Вас везде unsigned int. Почему ?


вопрос поставил в тупик.

O>Если работаете с памятью и особенно файлами, нельзя исключать ситуации с превышением 4GB.

O>Тут даже не size_t нужен (так как он будет эквивалентен unsigned int на x86), а что-то типа bigsize_t.
O>Кстати, компиляция под x64 подтверждает: "C4267 — conversion from <...> to <...>, possible loss of data".

для работы с файлом использовал __int64, максимальный размер буфера ограничил unsigned int. Хорошо, что Вы об этом написали — буду аккуратней.


O>3. В Index.cpp (line 42) есть такой фрагмент:

O>
O>GlobalMemoryStatusEx(&statex);
O>unsigned int avail_mem=(statex.ullTotalPhys>>32)?unsigned int(-1):(unsigned int)statex.ullAvailPhys;
O>

O>По-моему, так вычислять доступную память неправильно. Например, если 32-битная версия приложения
O>будет запущена на 64-битной Windows, она покажет всю доступную оперативку, хотя реально будет
O>доступно не более 2 гигов.

честно напишу — не вдавался в данную тему, изучу вопрос.

O>4. По коду, чисто субъективно. Вместо static я бы использовал anonymous namespaces, а из

O>fsort.h поубирал бы все, не имеющее прямого отношения к API. И макросы тоже бы выкинул, по возможности, в
O>том числе и те, которые используются для кодов ошибок и неймспейсов. Макросы — зло, почти всегда.

все typedef'ы и объявления не API функций вынести в отдельные .h файлы?

O>5. Ну и совсем IMHO — стиль оформления очень уж скомпрессированный.

O>Надо где-то и пробел вставлять иногда, и строку пустую.

все собираюсь изучить стиль Linux Kernel Coding Style, вероятно пришла пора.

O>Короче, если кое-где шлифануть, получится на добрую четверку.


Большое спасибо за ревью кода!
Re[2]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 09:25
Оценка:
Здравствуйте, acDev, Вы писали:

D>На беглый взгляд — очень адекватный кодес.

D>Может работодатели просто не способны понять как там всё работает. Или же просто goto "испугались"))

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

Спасибо за ревью!
Re[3]: ревью кода
От: DarkTranquillity  
Дата: 04.05.12 11:04
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

H>Здравствуйте, DarkTranquillity, Вы писали:


DT>>Единственное, что сразу бросается в глаза — это форматирование (принято в русском после знаков препинания и т.п. ставить пробелы), ну и как уже заметили, кое-где опущено почему-то RAII.


H>да, недоработка, буду исправляться.


DT>>В алгоритм не вникал, не понял почему он такой сложный. За распараллеливанием следите сами? Ну и число потоков естественно нет смысла делать больше числа процессоров.


H>вдохновлялся с coreutils::sort : файл бьется на сегменты заданного размера, каждый сегмент бьется на N частей и сортируется std::sort, после чего N сегметов дампятся во временный файл с использованием сортировки слиянием. В конце набор файлов попарно мержится сортировкой слиянием в новый файл, последний файл переименовывается в целевой.


H>Спасибо за ревью!

Это пожалуйста, покритиковать, как видите, все горазды.

Кстати, за четкое описание алгоритма уже 50% плюс, ИМХО. Не знаю, что у вас там за работодатели — боги С++, что ли?
Да, и скорость, скорость этого алгоритма действительно "быстрая по времени"?
Re[3]: ревью кода
От: okman Беларусь https://searchinform.ru/
Дата: 04.05.12 12:52
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

O>>Ну и передача структур аргументами функций, а-ля Win32 API — как-то не очень.


H>а как правильно?


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

O>>2. Я заметил, что типы-размеры у Вас везде unsigned int. Почему ?


H>вопрос поставил в тупик.


Этим целям лучше соответствует size_t, к тому же он при портировании кода на x64
позволяет избежать некоторых проблем.

H>все typedef'ы и объявления не API функций вынести в отдельные .h файлы?


Лучше их по возможности оставить в файлах реализации, чтобы они не видны были, где не нужно.
Re[4]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 13:35
Оценка:
Здравствуйте, DarkTranquillity, Вы писали:


DT>Кстати, за четкое описание алгоритма уже 50% плюс, ИМХО. Не знаю, что у вас там за работодатели — боги С++, что ли?


крупные софтверные компании в Новосибирске, в частности, вот это задание было от лаборатории касперского.

DT>Да, и скорость, скорость этого алгоритма действительно "быстрая по времени"?


ну вот на i5 M540 @2.53 116 метровый файл из цифр обрабатывается за 2700 мс в 4 потока без ограничения по памяти, уверен, что можно быстрей.
Re: ревью кода
От: Abyx Россия  
Дата: 04.05.12 13:44
Оценка: +1
Здравствуйте, Hydrophobia, Вы писали:

H>задание: реализовать алгоритм сортировки строк в файле за минимальное время.

H>zip-архив здесь: http://files.rsdn.ru/99432/file_sort.zip

Это С-style код с некоторыми фичами С++.
Код на С есть смысл писать только там где нельзя использовать С++ (некоторые микроконтроллеры, драйвера), сортировка строк в файле под виндой — это очевидно не та ситуация.
Как С++ код, это полное гуано. На С++ так не пишут — надо использовать умные указатели, RAII, никакого delete, и уж точно никакого goto cleanup

Есть несколько ошибок, например в "buffer_t*buffer=new buffer_t(in,conf.max_size_); if(!buffer) ..." Вы забыли "(std::nothrow)".
В "fsort_thread_arg_t*arg=new(std::nothrow) fsort_thread_arg_t;" nothrow есть, а вот проверки arg на NULL нет.
Вывод — nothrow тут не нужен, исключения как раз для таких ситуаций и придуманы.

По артихектуре кода:

Очень большие функции, скорей всего не соблюдается Single responsibility principle.
Комментарий "// merge temporary files" явно говорит о том, что тут должен быть вызов функции merge_temporary_files()

Мешанина из IO, многопоточности и собственно сортировки.
И конечно никаких юнит-тестов. Добавьте юнит-тесты, и архитектура сразу улучшится. (Именно юнит-тесты, а не интеграционные тесты).

Ну и прочие мелочи — arrowhead anti-pattern, закомментированный код.

Вместо __stdcall используйте WINAPI/CALLBACK, тогда будет собираться и под WinCE.
typedef struct в С++ не используют, т.к. незачем.
про size_t уже писали.
если есть buffer.cpp, то хорошо было бы иметь buffer.h(pp)

Непонятно в чем смысл FSORT_NAMESPACE_*
Также непонятно почему используется C IO вместо C++ IO или WinAPI (CreateFile/File Mapping). Есть подозрение что неблокирующее IO было бы быстрее.
In Zen We Trust
Re: ревью кода
От: CEMb  
Дата: 04.05.12 13:49
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

Код не смотрел, читал ревью.

Чисто так подскажу по теме delete, критических секций и прочий опасных моментов (почему по рукам за delete надо бить, не понял).
Инициализацию-деинициализацию опастных моментов можно помещать в конструктор-деструктор специальных под это дело классов. Т.о. при вылетах, выходах из зоны видимости мимо деинициализаторов и прочих форс-мажорах это будет перекрыто деструктором такого класса. Кроме того, код упростится, на одну строчку
Ну и да, надо пробелы между строчками-блоками и комменты по блокам ставить, читается быстро, смотрится гармонично.
Re[5]: ревью кода
От: CEMb  
Дата: 04.05.12 14:09
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

DT>>Кстати, за четкое описание алгоритма уже 50% плюс, ИМХО. Не знаю, что у вас там за работодатели — боги С++, что ли?

+1, кстати.

H>крупные софтверные компании в Новосибирске, в частности, вот это задание было от лаборатории касперского.


А ещё какие есть крупные конторы в Новосибирске?
Re[6]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 14:24
Оценка:
Здравствуйте, CEMb, Вы писали:

CEM>А ещё какие есть крупные конторы в Новосибирске?


Parallels
Re[2]: ревью кода
От: Hydrophobia  
Дата: 04.05.12 14:45
Оценка:
Здравствуйте, Abyx, Вы писали:


A>Есть несколько ошибок, например в "buffer_t*buffer=new buffer_t(in,conf.max_size_); if(!buffer) ..." Вы забыли "(std::nothrow)".

A>В "fsort_thread_arg_t*arg=new(std::nothrow) fsort_thread_arg_t;" nothrow есть, а вот проверки arg на NULL нет.
A>Вывод — nothrow тут не нужен, исключения как раз для таких ситуаций и придуманы.

хороший аргумент, учту.

A>Очень большие функции, скорей всего не соблюдается Single responsibility principle.

A>Комментарий "// merge temporary files" явно говорит о том, что тут должен быть вызов функции merge_temporary_files()

да, так и есть, разбиваются на мелкие только так.

A>Мешанина из IO, многопоточности и собственно сортировки.

A>И конечно никаких юнит-тестов. Добавьте юнит-тесты, и архитектура сразу улучшится. (Именно юнит-тесты, а не интеграционные тесты).

какую библиотеку можете посоветовать для юнит-тестов на С++ ? Какие книги есть по этой теме?

A>Непонятно в чем смысл FSORT_NAMESPACE_*

A>Также непонятно почему используется C IO вместо C++ IO или WinAPI (CreateFile/File Mapping). Есть подозрение что неблокирующее IO было бы быстрее.

мысль здравая, проверю на практике.

Спасибо за ревью!
Re[3]: ревью кода
От: Abyx Россия  
Дата: 04.05.12 15:27
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

H>какую библиотеку можете посоветовать для юнит-тестов на С++ ?

если есть буст, то есть смысл использовать boost.test
если нет буста, то можно использовать gtest
лучшей считается CATCH (https://github.com/philsquared/Catch/wiki), но она бета

Какие книги есть по этой теме?
достаточно прочитать туториал того же gtest,
хотя книг полно, например Working Effectively with Legacy Code, или The Art Of Unit Testing
In Zen We Trust
Re: ревью кода
От: maykie Россия  
Дата: 05.05.12 04:06
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

H>zip-архив здесь: http://files.rsdn.ru/99432/file_sort.zip


H>От Вас необходимы замечания и конструктивная критика кода.


В суть алгоритма не вникал из-за недостатка пробелов и комментариев. По мелочам.

out?fclose(out):false;

return result<0?true
result==0)?str0.length_<str1.length_
:false;

Грязь. Не надо так тернарный оператор абузить. Особенно как в первом случае.

__min(str0.length_,str1.length_)

Используй стандартный std::min вместо этого __уродства

if(argc<5)
return false;

Большая магическая константа. Да, ещё -f и -t не нужны.

wmain()
if(!parse_cmd(conf,argc,argv)){
fprintf(stderr,usage);
return 0;
}

Не возвращай 0 из main() если произошла ошибка. За это бьют по рукам.
0 означает, что команда выполнена успешно. Я, кстати, не увидел, где бы пользователи сообщалось что что-то случилось. Код возврата fsort игнорируется. Ну и зачем он нужен?

unsigned int __stdcall fsort_thread(void*param){
fsort_thread_arg_t*arg=(fsort_thread_arg_t*)param;
delete param;

за delete void* бить по рукам. Это undefined behaviour.

swprintf(fname,1023,L"%s%sfsort%i",temp_dir.c_str(),
(temp_dir.c_str()[temp_dir.length()-1]=='/' || temp_dir.c_str()[temp_dir.length()-1]=='\\')?L"":L"\\",
counter);

Грязь с магическими константами. То, что temp_dir заканчивается на / или \ надо было раньше проверять.

static unsigned int MAX_THREADS=8;


Мало. i7-980 поддерживает 12 потоков, к примеру.

typedef struct /*fsort_conf_t;*/{....
std::wstring directory_;// path to temporary directory

typedef struct /*temp_dir_*/{
std::wstring temp_dir_;

typedef struct /*cmd_conf_t*/{
std::wstring tmp_;

У тебя одна и та же сущность называется как минимум тремя разными именами.
Используй одно имя. При поддержке кода такая путаница в именах долбает по мозгам.

То, что ручное управление памятью — зло — говорили. Надо повторить, потому что это действительно зло.

Ну и ещё

private:
// deny
buffer_t(buffer_t const&)throw();
buffer_t&operator=(buffer_t const&)throw();


зачем говорить что они throw()?

Да, кстати, писать const справа это достаточно редкий стиль.

Вообщем эту мешанину c++, c(typedef struct) и lisp'а(многоэтажные expression'ы) тяжко читать.
Re[5]: ревью кода
От: DarkTranquillity  
Дата: 05.05.12 06:18
Оценка: -1
Здравствуйте, Hydrophobia, Вы писали:

H>Здравствуйте, DarkTranquillity, Вы писали:



DT>>Кстати, за четкое описание алгоритма уже 50% плюс, ИМХО. Не знаю, что у вас там за работодатели — боги С++, что ли?


H>крупные софтверные компании в Новосибирске, в частности, вот это задание было от лаборатории касперского.

Касперский обитает в Новосибе?? Вот уж не знал. Понятно, главные вирусописатели захотели сделать мегавирус, видимо.
Да они зажрались при выборе кадров.

DT>>Да, и скорость, скорость этого алгоритма действительно "быстрая по времени"?


H>ну вот на i5 M540 @2.53 116 метровый файл из цифр обрабатывается за 2700 мс в 4 потока без ограничения по памяти, уверен, что можно быстрей.

Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.