большие процедуры/функции

← →
young_dev (2011-03-30 17:47) [0]

написал процедуру, которая проверяет целостность содержимого определенного файла, получилось порядка 250 строк. нужно ли, с точки зрения организации кода, разбивать большие процедуры/функции? Хотелось бы услышать мнения по этому поводу.

← →
Медвежонок Пятачок © (2011-03-30 17:49) [1]

теперь все сотри что написал, и напиши функцию вместо процедуры.

← →
Дмитрий Тимохов (2011-03-30 17:50) [2]

смотря, что за процедура:
можно и не разбивать.
а можно и разбивать.
иногда нужно разбивать.
иногда просто обязательно.

← →
GanibalLector © (2011-03-30 17:51) [3]

"Совершенный код" Макконнелл

Там есть ответ!

← →
Медвежонок Пятачок © (2011-03-30 17:53) [4]

нужно ли, с точки зрения организации кода

у организации кода нет точки зрения. значит не нужно.

← →
clickmaker © (2011-03-30 17:54) [5]

> нужно ли, с точки зрения организации кода, разбивать большие

Если в процессе написания большой п/ф возникает дежа вю, то надо

← →
Kerk © (2011-03-30 17:54) [6]

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

Вместо

function CheckFile: Boolean; begin // Check Header ... 200 строк // Check Body ... 200 строк // Check Footer ... 200 строк end;
лучше будет смотреться
function CheckFile: Boolean; begin CheckHeader; CheckBody; CheckFooter; end;

Если у тебя там есть комментарии (а в 250-строчной функции они скорее всего есть), то это признак того, что код стоит переписать.

← →
Dennis I. Komarov © (2011-03-30 17:55) [7]

> которая проверяет целостность содержимого определенного
> файла, получилось порядка 250 строк

все может быть, но что-то я сомневаюсь в их нужности...

> нужно ли, с точки зрения организации кода, разбивать большие
> процедуры/функции?

зависит от содержимого

← →
Игорь Шевченко © (2011-03-30 17:56) [8]

> лучше будет смотреться

Спорный вопрос

← →
Kerk © (2011-03-30 17:59) [9]

> Игорь Шевченко © (30.03.11 17:56) [8]
>
> > лучше будет смотреться
>
> Спорный вопрос

Ну почему же. Здесь дело не только в группировке кусков кода, но и в напоминании себе (а компилятор проконтролирует), что эти куски логически разделены. С каждым таким куском уже можно работать отдельно, не задумываясь о том, что там выше или ниже будет происходить в данной области видимости.

← →
Игорь Шевченко © (2011-03-30 18:06) [10]

> Ну почему же.

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

← →
Kerk © (2011-03-30 18:10) [11]

> Игорь Шевченко © (30.03.11 18:06) [10]
>
> > Ну почему же.
>
> потому что в каждом случае удобно делать в зависимости от
> случая. В ряде случаев код становится менее удобочитаемым
> при подобной разбивке.

Ну это понятно. То были просто общие соображения.

← →
Думкин (с отпуска) (2011-03-30 18:11) [12]

> Kerk © (30.03.11 17:59) [9]

У меня сейчас взаимонепонимание с "тем кто выше". Как раз тут. Он хочет архитекрутить и т.п. Начитался. Бывает.

В итоге простая операция, иногда, превращается, в дикий предподвыверт, лишь бы соответствовать его представлениям "О". Я думаю о смене места работы.

← →
Думкин (с отпуска) (2011-03-30 18:14) [13]

> В ряде случаев код становится менее удобочитаемым при подобной
> разбивке.

Вот. ИНе логичнмы, что еще грустнее.

← →
Kerk © (2011-03-30 18:27) [14]

> Думкин (с отпуска) (30.03.11 18:11) [12]

Любую идею можно довести до абсурда. Чего накинулись? :)

← →
young_dev (2011-03-30 18:31) [15]

Примерно такая процедура.

procedure Items.analyze_file_of_import(const file_name: string; full_analyze: Boolean); var // ... I, prod_status, category_id : Integer; // ... begin // ... for I := 0 to Self.Count - 1 do begin //... // caheck value of field prod_status Fprod_status := Vals[C_PROD_STATUS_NAME]; if not (prod_status in [C_PROD_STATUS_AVAILABLE.. C_PROD_STATUS_NOT_AVAILABLE]) then raise Exception.CreateResFmt(@C_INVALID_PROD_STATUS, [Fprod_status, I]); // check value of field category_id Fcategogy_id := Vals[C_CATEGORY_ID_NAME]; if not is_valid_category(Fcategory_id) then raise Exception.CreateResFmt(@C_INVALID_CATEGORY_ID, [Fcategory_id, I]); // ... // еще порядка 30 аналогичных проверок, код получается около 250 строк // ... end; // ... end;

может для проверки каждого поля сделать отдельные функции, типа:

check_prod_status_val();
check_category_id_val();

← →
Kerk © (2011-03-30 18:33) [16]

В твоем случае похоже и правда нет смысла разбивать это. Лучше не станет. Подумай о том, как можно унифицировать проверки. Если никак, то судьба, значит.

← →
Kerk © (2011-03-30 18:34) [17]

> // check value of field category_id

Такие комментарии в общем-то бессмысленны. И без них видно, что происходит какая-то проверка. А вот в чем смысл проверки - вопрос :)

← →
young_dev (2011-03-30 18:46) [18]

> А вот в чем смысл проверки - вопрос

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

← →
Думкин (с отпуска) (2011-03-30 18:58) [19]

> young_dev (30.03.11 18:31) [15]

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

Класс там какой и т.п. И вообще, бррр...

> Kerk © (30.03.11 18:27) [14]
> Любую идею можно довести до абсурда. Чего накинулись? :)

Так доводят же. В реальности. Маконнелла прочитают и начинают добро направо и налево раздавать.

← →
Игорь Шевченко © (2011-03-30 19:10) [20]

> начинают добро направо и налево раздавать.

Чтоб никто не ушел обиженным. Ампутация рук по самую голову очень хорошо помогает в таких случаях.

← →
Pavia © (2011-03-30 19:17) [21]

Судя по всему функция останется такой длиной. Но вот для удобства восприятия можно организовать её в виде таблички.

chek(C_PROD_STATUS_NAME, @check_prod_status_val, @C_INVALID_PROD_STATUS);
chek(C_CATEGORY_ID_NAME, @check_category_id_val, @C_INVALID_CATEGORY_ID);

← →
TUser © (2011-03-30 21:04) [22]

Личное правило: функция должна умещаться на экране. Поэтому экран надо побольше. Но 250 строк все равно надо разбить. Разбил бы я сам или нет - сильно зависит от фазы луны.

← →
Rimma (2011-03-30 21:19) [23]

Лучшее враг хорошего!

← →
palva © (2011-03-30 22:09) [24]

> может для проверки каждого поля сделать отдельные функции

А что, нормальная идея. Особенно, если в дальнейшем планируется добавление новых полей, расширение допустимых диапазонов для отдельных полей... Даже если не планируется, это часто приходится делать.

← →
_Юрий (2011-03-30 22:45) [25]

Если все строго сверху вниз без длинных ветвлений, то разбивать необязательно, можно отформатировать и по другому. Регионами например.

← →
DVM © (2011-03-30 23:23) [26]

> лучше будет смотреться
>
> function CheckFile: Boolean;
> begin
> CheckHeader;
> CheckBody;
> CheckFooter;
> end;

в новых делфи есть такая удобная вещь как регионы - свернуть можно любой кусок кода и все смотреться будет отлично

← →
картман © (2011-03-30 23:30) [27]

> Так доводят же. В реальности. Маконнелла прочитают и начинают
> добро направо и налево раздавать.

> young_dev (30.03.11 18:31) [15]
>
> Примерно такая процедура.
>

> // еще порядка 30 аналогичных проверок, код получается
> около 250 строк

все просто: 30 классов и фабрика

← →
Rouse_ © (2011-03-31 01:08) [28]

> Kerk © (30.03.11 18:34) [17]
> Такие комментарии в общем-то бессмысленны. И без них видно,
> что происходит какая-то проверка.

Да, действительно, зачем?
Открываю первый попавшийся исходник винды, ну например реализацию FLoadDynEmRules, читаю:

/* F L O A D D Y N E M R U L E S */ /*---------------------------------------------------------------------------- %%Function: FLoadDynEmRules %%Contact: daleg Load rules dynamically from a file or a data structure, depending upon #ifdef ----------------------------------------------------------------------------*/ #ifdef DYN_RULES_FROM_STRUCT #include "emruli.oci" #endif /* DYN_RULES_FROM_STRUCT */ int FLoadDynEmRules(void) { int docii; #ifndef DYN_RULES_FROM_STRUCT /* Read opcodes from a disk file */ if (!MsoFReadDynOciRules("em", &docii)) return fFalse; #else /* DYN_RULES_FROM_STRUCT */ /* Get opcodes from a structure */ vlpruls->pociiDynRules = (PV) vrgociiEm; docii = IMaxRg(vrgociiEm, MSOOCII); vlpruls->rgpchDynNames = _rgszEmRulNames; #endif /* !DYN_RULES_FROM_STRUCT */ #define NON_CONST_OCI_VTABLE #ifdef NON_CONST_OCI_VTABLE /* Insert Mso functions into OCI vtable */ if (!MsoFCopyBaseRulRgpfnoci(vpfnociEm)) return fFalse; #endif /* NON_CONST_OCI_VTABLE */ /* Generate rulebase nodes from opcode stream */ return MsoFLoadDynRulesPocii(vlpruls->pociiDynRules, docii, vpfnociEm, vrgocadEm, vrgcbOciArgEm, IMaxRg(vpfnociEm, MSOPFNOCI), DebugElse(vlpruls->rgpchDynNames, pNil)); }

Такие комментарии в общем-то бессмысленны, и без них видно что происходит, но... они есть. Наверное не просто так?

> Rouse_ © (31.03.11 01:08) [28]

Ну если ты не видишь принципиальной разницы между комментариями типа
// caheck value of field prod_status // check value of field category_id
и
/* Read opcodes from a disk file */ /* Generate rulebase nodes from opcode stream */
то я ничем не могу помочь.

> Kerk © (31.03.11 01:13) [29]

Есть комментарии и комментарии. И главное в комментариях - это полезность самому автору. (Если он не работает в команде или если его код другие участники команды не смотрят в виду отсутствия производственной необходимости).
Другое дело если требования к комментариям как-то зафиксировано руководителем работ.

> Германн © (31.03.11 01:54) [30]
>
>

Да. Забыл указать ещё один вариант. Если речь идет о компоненте/библиотеке, которые поставляются (с исходниками) другим людям, то, конечно, комментарии должны следовать другим правилам.

← →
han_malign (2011-03-31 09:12) [32]

> check_prod_status_val();
> check_category_id_val();

> Держать в цикле такой кошмар мне моя отсутствующая религия - не позволяет.

я бы такую схему предложил для начала
function validate(vals: array of TMyVals, var failField: TMyValEnum): boolean; begin Result:= false; failStage:= C_PROD_STATUS_NAME; Fprod_status := Vals[failField]; if not (prod_status in [C_PROD_STATUS_AVAILABLE..C_PROD_STATUS_NOT_AVAILABLE]) then exit; failField:= C_CATEGORY_ID_NAME; ................... Result:= true; end;
...
for I := 0 to Self.Count - 1 do if( not validate(vals, stage) )then raise Exception.CreateResFmt(rgsErrorMsgFmts[stage], [vals[stage], I]);

затем, в контексте Pavia © [21], подумал бы о (при условии что нет проверок контекстно зависимых от других полей)
function validateField(Field: TValEnum; const Value): boolean; begin Result:= false; case(Field)of C_PROD_STATUS_NAME: Result:= ...; ... else raise Exception.CreateResFmt("Забыл добавить ветвление для %d", [Field]); end{case(Field)}; end{function validateField};

теперь самой длинной(но уже раза в два короче) функцией будет validateField, зато применяться она может в любом месте - включая заполнение полей пользователем - а вся остальная логика становится короткой и ясной...

← →
han_malign (2011-03-31 09:17) [33]

(продолжение):
for I := 0 to Self.Count - 1 do for stage:= low(stage) to high(stage) do if not validateField(stage, vals[stage])then raise Exception.CreateResFmt(rgsErrorMsgFmts[stage], [vals[stage], I]);

Не бывает больших процедур, бывает маленькое разрешение экрана :) А вообще - разбиение кода на процедуры - это палка о двух концах, т.к. нужно учитывать еще такое понятие как связность кода.

← →
han_malign (2011-03-31 09:30) [35]

> такое понятие как связность кода

- side-эффекты(не знаю как этот термин на русский перевести) глобального контекста(тут забыл, там недоперепрятал)...

З.Ы. А целостность полей лучше проверять и подсвечивать при вводе...

> З.Ы. А целостность полей лучше проверять и подсвечивать
> при вводе...

Марксизм не догма, а руководство к действию

> Kerk © (31.03.11 01:13) [29]
> Ну если ты не видишь принципиальной разницы между комментариями

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

> > может для проверки каждого поля сделать отдельные функции

> А что, нормальная идея. Особенно, если в дальнейшем планируется
> добавление новых полей, расширение допустимых диапазонов
> для отдельных полей...

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

Ещё несколько лёгких движений напильником, и готов самопильный датасет.
Вот только остаётся не при делах регенерирующий реактор на субтепловых нейтронах.

Удалено модератором

большие процедуры/функции Найти похожие ветки