ревью кода
От: 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-символов?
Re[6]: ревью кода
От: Hydrophobia  
Дата: 05.05.12 07:49
Оценка:
Здравствуйте, DarkTranquillity, Вы писали:


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

DT>Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?

длина строки в тестовом файле до 1000 символов, работает только с ASCII. На Unicode/UTF-8 не пробывал расширять.
Re[7]: ревью кода
От: Hydrophobia  
Дата: 05.05.12 08:02
Оценка:
Здравствуйте, DarkTranquillity, Вы писали:


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

DT>Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?

Если использовать в сравнении строк memcmp(), то время работы не ухудшится, если использовать WideCharToMultibyte()/MultiByteToWideChar() + wcsncmp(), то время увеличится.
Re[2]: ревью кода
От: Hydrophobia  
Дата: 05.05.12 08:24
Оценка:
Здравствуйте, maykie, Вы писали:

M>

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

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

консольная оболочка — только для тестов алгоритма во время разработки. каюсь, нужны были юнит-тесты.

M>

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

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

если бы передавался указатель на класс, то да, однозначно плохо, для С-структур — вполне.

M>

M>static unsigned int MAX_THREADS=8;


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


пора апгрейды покупать.

M>

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


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

я за то, что бы конструктор, конструктор копирования, деструктор и оператор присваивания не генерировали исключений. Но в связи с последними комментариями, такое не всегда можно сделать, особенно, если политика обработки ошибок базируется на исключениях. Вроде Саттер писал что-то по этому поводу в одной из "Эффективное использование C++".

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


Так понятней к чему const относится.

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

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



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

DT>>Хм.. Неплохо. Интересно, а если увеличить длину строки, и добавить в нее всяких экзотических не ASCII-символов?

H>Если использовать в сравнении строк memcmp(), то время работы не ухудшится, если использовать WideCharToMultibyte()/MultiByteToWideChar() + wcsncmp(), то время увеличится.


Вы просто капитан Очевидность!
Использовать простое копирование памяти, или вызовы трех не столь нетривиальных функций.
Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.
Re[9]: ревью кода
От: Hydrophobia  
Дата: 05.05.12 09:07
Оценка:
Здравствуйте, DarkTranquillity, Вы писали:

DT>Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.


в смысле лексикографический порядок?
Re[10]: ревью кода
От: DarkTranquillity  
Дата: 05.05.12 10:06
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

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


DT>>Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.


H>в смысле лексикографический порядок?


Да.
Re[11]: ревью кода
От: Hydrophobia  
Дата: 05.05.12 10:17
Оценка:
Здравствуйте, DarkTranquillity, Вы писали:

DT>>>Вопрос в другом — при сравнении строк ведь есть еще такое понятие как collate.


H>>в смысле лексикографический порядок?


DT>Да.


по хорошему, он должен быть указан в задаче. Тот же sort из coreutils имеет параметры, задающие те или иные правила сравнения (--general-numeric-sort, --ignore-nonprinting, --ignore-case и т.д.).
Re: ревью кода
От: Nikе Россия  
Дата: 21.06.12 12:47
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

Посмотрел fsort с точки зрения чистоты кода. Если код писался на время — то это не существенно, если на качество — существенно.

Функция fsort очень длинная. По хорошему, функции должны быть на пол экрана, не больше. Каждая функция должна выражать одну идею и быть однородной по составу. Название функции должно выражать то что она делает, так чтобы не нужно было заглядывать внутрь, чтобы вскрывать нюансы.
Больше автоматики, HANDLE и голые указатели это прошлый век.

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

>unsigned int fsort(std::wstring const&in,std::wstring const&out,fsort_conf_t const&conf){
тытекстытожепишешьбезпробелов?

>buffer_t*buffer=new buffer_t(in,conf.max_size_);

потенциальный меморилик + необходимость тратить ценное внимание на контроль

>std::vector<HANDLE>

в общем случае лучше использовать автоматику

на мой взгляд именования классов с _t стоит использовать когда косишь под стандарт (но это субъективно)

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

Я бы написал if, такие вещи нужно использовать только в очень простых случаях.

Смысл использования FSORT_NAMESPACE_BEGIN не очевиден и в примере ИМХО это лишнее.

>typedef struct{

>} merge2files_thread_arg_t;
Смысл писать такие вещи в 21 веке?

>#include <windows.h>

Лучше не использовать в хидере. Как же потенциальная кроссплатформенность?

>unsigned int __stdcall fsort_thread(void*param);

>unsigned int __stdcall merge2files_thread(void*param);
смысл этого объявления?
Нужно разобрать угил.
Re[3]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 12:57
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

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


H>если бы передавался указатель на класс, то да, однозначно плохо, для С-структур — вполне.


Нет. Помимо вызова деструктора delete может использовать информацию о размере объекта.

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


H>Так понятней к чему const относится.


Мне _const нравится больше, но обычно он не используется, к сожалению.
Нужно разобрать угил.
Re: ревью кода
От: Glas  
Дата: 21.06.12 13:00
Оценка: -1
Здравствуйте, Hydrophobia, Вы писали:

1. goto? Вы не на ассемблере пишите, чтобы таким пользоваться, я бы уже только за это ваш проект в мусорку выкинул.
2. Где комментарии в функциях. Есть комментарии к функциям, но нет комментариев в телах функций.
3. Не понял зачем namespace объявлены.
Re[2]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 13:07
Оценка: +1
Здравствуйте, Glas, Вы писали:

G>2. Где комментарии в функциях. Есть комментарии к функциям,

Кстати — зря, лучше бы более читаемое название функций было сделано.

G>но нет комментариев в телах функций.

Они вредны.
Нужно разобрать угил.
Re[2]: ревью кода
От: java_coder771  
Дата: 21.06.12 13:11
Оценка: -2
A>Это С-style код с некоторыми фичами С++.
A>Код на С есть смысл писать только там где нельзя использовать С++ (некоторые микроконтроллеры, драйвера), сортировка строк в файле под виндой — это очевидно не та ситуация.
A>Как С++ код, это полное гуано. На С++ так не пишут — надо использовать умные указатели, RAII, никакого delete, и уж точно никакого goto cleanup


знаешь, интервьюверы нередко (именно в майкрософт и еще нескольких крупных конторах) просят на собеседовании
писать код на простом C, чтобы понять суть алгоритма и не отвлекаться на непонятные синтаксические конструкции
Re[3]: ревью кода
От: Glas  
Дата: 21.06.12 13:16
Оценка:
Здравствуйте, Nikе, Вы писали:

G>>но нет комментариев в телах функций.

N>Они вредны.

Они могут быть вредны в очевидных местах и в непосредственной разработке. В данном случае заказчику надо быстро и внятно показать, что делает код или пусть как хочет разбирается?
Re[4]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 13:21
Оценка: +1
Здравствуйте, Glas, Вы писали:

G>>>но нет комментариев в телах функций.

N>>Они вредны.

G>Они могут быть вредны в очевидных местах и в непосредственной разработке. В данном случае заказчику надо быстро и внятно показать, что делает код или пусть как хочет разбирается?


Я исхожу из правила, что необходимость комментария в коде говорит нам о необходимости рефакторинга.
Можно написать комментарий. При этом код будет продублирован и по сути станет сложнее, так как появится необходимость поддерживать не только плохой код, но и комментарий.
Хороший рефакторинг — упростит ситуацию и сделает суппорт дешевле.
Нужно разобрать угил.
Re[5]: ревью кода
От: Glas  
Дата: 21.06.12 13:28
Оценка: -1
Здравствуйте, Nikе, Вы писали:

N>Можно написать комментарий. При этом код будет продублирован и по сути станет сложнее, так как появится необходимость поддерживать не только плохой код, но и комментарий.

Как это комментарий дублирует код?
ЗЫ всегда думал, что комментарии нужны, чтобы другие программисты быстрее понимали, как это работает.
Re: ревью кода
От: vpchelko  
Дата: 21.06.12 13:31
Оценка:
Я где-то уже это слышал, очень много кода...

Логичный отлуп получил.
Сало Украине, Героям Сала
Re[6]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 13:32
Оценка: +1
Здравствуйте, Glas, Вы писали:

N>>Можно написать комментарий. При этом код будет продублирован и по сути станет сложнее, так как появится необходимость поддерживать не только плохой код, но и комментарий.

G>Как это комментарий дублирует код?
Код описывает функциональность, комментарий описывает функциональность. Дубль.
Код должен понятно описывать функциональность. Конкретно С++ даёт множество инструментов для этого. Если код это не делает — то это плохой код и его нужно делать хорошим, а не добавлять костыли.

G>ЗЫ всегда думал, что комментарии нужны, чтобы другие программисты быстрее понимали, как это работает.

Для ускорения понимания нужно добавлять примеры и высокоуровневое описание структуры. А для того чтобы понять как работает код — надо читать код, никак иначе.
Нужно разобрать угил.
Re[6]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 13:37
Оценка: +1
Здравствуйте, Glas, Вы писали:

G>Как это комментарий дублирует код?


Два банальных примера плохого использования комментариев:
Комментарий в объявлении:
// merge 2 blocks into one
void merge2blocks(std::vector<line_t> const&block0,std::vector<line_t> const&block1,std::vector<line_t>&result);

Дубль + возможно не очень хорошее именование.

Комментарий в cpp:
  // merge temporary files

Ясный детектор необходимости новой функции: MergeFiles( temporary ).
Нужно разобрать угил.
Re: ревью кода
От: Nikе Россия  
Дата: 21.06.12 13:40
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

Ещё:
рекомендую использовать: typedef std::vector<Line> Lines; а не совать повсюду std::vector<line_t>, так как:
1. короче и понятнее (дано имя).
2. если захочется поменять контейнер, добавить аллокатор, прорефакторить — будет проще это сделать.
Нужно разобрать угил.
Re: ревью кода
От: Handie  
Дата: 21.06.12 14:03
Оценка: +1
Код даже комментить не хочу, по нему можно трактат написать как не надо программировать.

Вообще-то Вы реализовали unix sort. Надо бы объяснить в комментах какой алгоритм использован и почему. Код неимоверно сложен. Парсинг командной строки ручками — это такое извращение? Или stdarg.h уже в винде отменили? Я например не понял, зачем нужна многопоточность и как она соотносится с блокирующими операциями чтения диска.

Все жутко перемешано вместе. Алгоритмика должна быть отдельно, ввод/вывод отдельно. Очень плохо структурированы модули и невнятные интерфейсы между ними. STL используется не по делу. Мешанина из типов Win32/STL/C

Надо сделать куда проще и понятней.
Re[7]: ревью кода
От: Glas  
Дата: 21.06.12 14:06
Оценка:
Здравствуйте, Nikе, Вы писали:
G>>Как это комментарий дублирует код?

N>Два банальных примера плохого использования комментариев:

N>Комментарий в объявлении:
N>
N>// merge 2 blocks into one
N>void merge2blocks(std::vector<line_t> const&block0,std::vector<line_t> const&block1,std::vector<line_t>&result);
N>

N>Дубль + возможно не очень хорошее именование.
А это как раз из области очевидного. Я тут даже только ЗА правильное наименование функции и аргументов. А если совсем уж копнуть то

block_t*result=new(std::nothrow) block_t;
    if(result){
      result->reserve(block0->size()+block1->size());
      merge2blocks(*block0,*block1,*result);
      delete block0;
      delete block1;
      blocks.push(result);
    }


Заменить на

std::vector<line_t> merge2blocks(std::vector<line_t> const&block0,std::vector<line_t> const&block1);


      block_t*result = merge2blocks(*block0,*block1);
      delete block0;
      delete block1;
      if(result){
      blocks.push(result);
    }


Все подготовки будут внутри функции, и тогда понятно что мы сливаем два блока в один, который и возвращается из функции.
Re[8]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 14:17
Оценка:
Здравствуйте, Glas, Вы писали:

G>
G>std::vector<line_t> merge2blocks(std::vector<line_t> const&block0,std::vector<line_t> const&block1);
G>


Lines Merge( Lines const& block0, Lines const& block1 );

Хотя и не очевидна допустимость возврата вектора.
Нужно разобрать угил.
Re[9]: ревью кода
От: Glas  
Дата: 21.06.12 14:22
Оценка:
Здравствуйте, Nikе, Вы писали:

Я на сеньора по С++ не мечу, так что можно и моим вариантом
Re[3]: ревью кода
От: Abyx Россия  
Дата: 21.06.12 14:30
Оценка:
Здравствуйте, java_coder771, Вы писали:

A>>Это С-style код с некоторыми фичами С++.

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


_>знаешь, интервьюверы нередко (именно в майкрософт и еще нескольких крупных конторах) просят на собеседовании

_>писать код на простом C, чтобы понять суть алгоритма и не отвлекаться на непонятные синтаксические конструкции

непонятные кому?
In Zen We Trust
Re[4]: ревью кода
От: java_coder771  
Дата: 21.06.12 14:57
Оценка:
A>непонятные кому?

непонятные интервьюверу
Re: ревью кода
От: Геннадий Васильев Россия http://www.livejournal.com/users/gesha_x
Дата: 21.06.12 16:59
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

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


На вскидку:

1. Приличные люди кладут тестовый пример с данными и .bat-файлом.

2. Дичайшая помесь С-style и С++. Например, вот это — форменный абсурд:

typedef struct{
  unsigned int max_size_;// max size in bytes
  unsigned int max_threads_;// max threads
  std::wstring directory_;// path to temporary directory
} fsort_conf_t;


typedef для структур нужен только для С (и если мне не изменяет ПЗУ, то лишь для достаточно древних компиляторов). Но std::wstring можно вставить только в код на C++, соответственно, typedef тут — явное излишество.

Потом, это, конечно, круто, что ты знаешь, что есть такая штука, как (std::nothrow), но вот ведь беда — std::vector об этом может и не догадываться, т.е. от исключений ты на самом деле не защищён. Соответственно, скажем, конструкция:
arg->files_.push(out_fname);
запросто может плюнуть исключение, которое не будет перехвачено внутри функции потока (merge2files_thread), то есть ускачет куда-то в системные обработчики.

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

4. Какой смысл в макросах FSORT_NAMESPACE_BEGIN/FSORT_NAMESPACE_END ? Ты собираешься поддерживать компиляторы, не поддерживающие namespace? Тогда нельзя будет юзать и std::wstring.

5. Это что за зверь?

  file_?fclose(file_):false;


Трудно написать так:

  if (file_){
    fclose(file_);
  }


?

6. Вот такие вещи из демонстрашек надо убирать:

//*end=0;// remove


7.
#if !defined _FSORT_H_
#define _FSORT_H_ 1


Это что за умничанье на ровном месте?

Ну вот так вот, примерно. В конечном итоге код на самом деле отпугивает безотносительно использованного алгоритма.
Я знаю только две бесконечные вещи — Вселенную и человеческую глупость, и я не совсем уверен насчёт Вселенной. (c) А. Эйнштейн
P.S.: Винодельческие провинции — это есть рулез!
Re[3]: ревью кода
От: Геннадий Васильев Россия http://www.livejournal.com/users/gesha_x
Дата: 21.06.12 17:17
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

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


Вот это как раз и нужно было написать в комментариях.
Я знаю только две бесконечные вещи — Вселенную и человеческую глупость, и я не совсем уверен насчёт Вселенной. (c) А. Эйнштейн
P.S.: Винодельческие провинции — это есть рулез!
Re[4]: ревью кода
От: Nikе Россия  
Дата: 21.06.12 18:03
Оценка:
Здравствуйте, Геннадий Васильев, Вы писали:

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


ГВ>Вот это как раз и нужно было написать в комментариях.


На английском.
Нужно разобрать угил.
Re: ревью кода
От: Паблик Морозов  
Дата: 21.06.12 21:32
Оценка:
Здравствуйте, Hydrophobia, Вы писали:

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


Может стоит попробовать использовать нормальный язык программирования?
 
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.