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...
Пока на собственное сообщение не было ответов, его можно удалить.