Ужас! Дожились...

← →
Alex Konshin © (2005-09-16 10:41) [40]

Насчет C++ я погорячился, но и в Delphi так писать не стоит. Если уж используем with, то почему не использовать его для упрощения этих конструкций. Ну или очевидное решение - локальные переменные.

← →
msguns © (2005-09-16 10:44) [41]

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

← →
Игорь Шевченко © (2005-09-16 10:46) [42]

msguns © (16.09.05 10:44) [41]

> Он говорил: если один оператор не умещается в одну строку
> на экране - это плохой оператор.
> Немного, конечно, перегнул

Я не вижу вообще никакого перегиба. Он прав целиком и полностью

← →
Igorek © (2005-09-16 10:51) [43]

> Я не вижу вообще никакого перегиба. Он прав целиком и полностью

← →
Desdechado © (2005-09-16 13:27) [44]

Alex Konshin © (16.09.05 04:43) [38]
> Эту страшилку ты сам себе придумал.
msguns © (16.09.05 10:00) [39]
> Это не страшилка, это уродство
Во-первых, я не утверждал, что этот код мой. Но стиль мне близок и понятен.

> У тебя там несколько раз вычисляется одни и те же объекты.
> Если эти вычисления делать всего лишь раз и завести временые
> переменные, то и код становится совершенно безобидным.
Не вижу причины заводить локальную переменную только для того, чтобы 1 раз ей что-то присвоить. Да и обращение по адресу в этих сравнениях тоже выполняется быстро. А если учесть, что первое из условий AND практически всегда FALSE, и включен короткий разбор условий, то накладных расходов минимум. Код, имхо, вполне читабельный, т.к. в строке видно всю иерархию зависимостей, и нет надобности лезть куда-то еще в поисках временной переменной для понимания, что же в ней такое есть.

> То есть, налицо просто незнание возможностей языка
Каких именно? Примерчик оптимизации этого куска хотелось бы видеть (я не о временных переменных).

> если один оператор не умещается в одну строку на экране - это плохой оператор
Стоит переписать имена классов и переменных в однобуквенные, и все поместится :)
Только вот станет совершенно нечитабельным. Хуже фортрана...

← →
Alex Konshin © (2005-09-16 13:42) [45]

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

Один раз присвоить и три раза использовать. Оптимизация вполне имеет смысл даже с точки зрения времени исполнения, я уж не говорю о том, что код будет более читабельным. И какие проблемы с тем, чтобы завести временую переменную? Религия не позволяет?

← →
Desdechado © (2005-09-16 14:04) [46]

А ты для вот такого:
datamodule.qry.fieldbyname( aFields[ i ][ j ].Name ).AsInteger
Тоже отдельную переменную заводишь? Смысл тот же ведь.

Ведь тогда программа превратится в сборище локальных переменных, которые никакой логикой человеческой не разгребешь! Потому как или их будет много на каждую такую "длинную" строчку кода, либо они будут повторно использованы (ужас!), либо друг на друга ссылаться с глубокой иерархией. Смысл такой оптимизации? Скорее, только хуже будет.

А про незнание возможностей языка вопрос остался открытым.

← →
Игорь Шевченко © (2005-09-16 14:06) [47]

> datamodule.qry.fieldbyname( aFields[ i ][ j ].Name ).AsInteger

Немецкий язык. Сделай метод у datamodule

← →
Desdechado © (2005-09-16 15:30) [48]

> Сделай метод у datamodule
Это просто пример. Но не факт, что метод здесь ВСЕГДА подойдет. Спагетти ложкой есть неудобно. Поэтому категоричные заявления некоторых коллег должны сопровождаться условиями применения их советов. Ведь советов на все случаи жизни не бывает.

← →
MeF Dei Corvi © (2005-09-16 15:45) [49]

> Ведь тогда программа превратится в сборище локальных переменных,
> которые никакой логикой человеческой не разгребешь!

Откуда там сборище? Две-три локальные переменные или несколько ф-ий этот код испортить не смогут...

← →
msguns © (2005-09-16 15:49) [50]

>Desdechado © (16.09.05 14:04) [46]
>А ты для вот такого:
>datamodule.qry.fieldbyname( aFields[ i ][ j ].Name ).AsInteger
>Тоже отдельную переменную заводишь? Смысл тот же ведь.

Зачем это уродство:
"aFields[ i ][ j ].Name"
А нельзя использовать нормальную конструкцию с переменной FName, определенной строкой ранее ?

Если же ты хочешь перебрать все поля записи из некоторого заранее неизвестного (в смысле по размеру и содержанию) массива имен полей (типа универсальность и кульность) и их значения чему-то там присвоить, то возникает недоумение по поводу однотипности поля этого "универсального" НД.
Да и вообще это не так делается. Если ты хочешь построить что-то в стиле объектной БД, то надо не с массивами трахаться а определить сами объекты БД как классы со всеми методами. А если по уму, то сами классы вшить в таблицы же, откуда и загружать при инициализации БД.
Тогда и обращение с такими объектами (код в программе-клиенте) будет краткое и прозрачное.

То же, что ты привел, не просто уродство - это преступление. Что будет,
если изменения, сделанные в самой БД, не "впишутся" в твой "универсальный" алгоритм ? Переписывать все нафиг ? А за то, что корректить подобный код все равно, что отобедать калом, кто ответит ?

← →
Desdechado © (2005-09-16 16:27) [51]

MeF Dei Corvi © (16.09.05 15:45) [49]
> Две-три локальные переменные код испортить не смогут
Взгляни на обсуждаемый пост [21] и посчитай, сколько там будет переменных, необходимых до провозглашенной красоты (чтобы строчка кода была одна). А теперь представь, что это фрагмент других подобных обработок, которые отличаются только индексом в самом глубоком Items (и не только).

msguns © (16.09.05 15:49) [50]
нельзя все так близко к сердцу принимать :)
Это всего лишь пример "длинной" строки кода, не несущий никакого смысла, кроме самой длины. Поэтому догадки об объектах в БД и прочего излишни.
Просто я хотел сказать, что не всегда имеет смысл заменять "длинную" конструкцию обращения к какому-то полю класса присваиванием временной переменной. Ведь так можно дойти и до элементарных операций, типа:
a:=0;
b:=1;
c:=a+b;
d:=c*2;
e:=sin(d);
f:=round(e);
g:=f/4;
Красиво, но ни фига не понятно. И бессмысленно.

> корректить подобный код все равно, что отобедать калом
ай, не гоже!

← →
Sandman29 (2005-09-16 16:44) [52]

Хотя бы вот так, хотя код еще не оптимальный...
var
ItemsI: TList;
...
with( liLevel ) do
begin
ItemsI := TList(Items[ i ]);
if( {--- зубья вилки похожи? ---}
( TChainPoint( Items[ i - 1 ] ).ContactorContainernObjEqualsAndContactorObjEquals(
TChainPoint( TList( ItemsI.Items[ j ]).Items[ k ] )))
and
{--- указанная точка - ручка вилки? ---}
( oPoint.ContactorContainernObjEqualsAndContactorObjEquals(
TChainPoint( ItemsI.Items[ j - 1 ] ))) and
( oPoint.nCont =
TChainPoint( ItemsI.Items[ j - 1 ]).nCont ) )

← →
msguns © (2005-09-16 16:55) [53]

Ладно, не будем "цепляться к словам".
Вместо
datamodule.qry.fieldbyname( aFields[ i ][ j ].Name ).AsInteger
напишем
datamodule.qry.fieldbyname("Field1").AsInteger

и попробуем проверить его на предмет смысла

В операторе, состоящей из 5 слов, видим:
-имя объекта доступа к БД
-место определения этого объекта
-имя поля датасета, к которому обращаемся
-тип поля датасета.

Итого 4 единицы информации на 5 слов

Теперь берем твой оператор:

if( {--- зубья вилки похожи? ---}
( TChainPoint( Items[ i - 1 ] ).pContactor.pContainer.nObj =
TChainPoint( TList( TList( Items[ i ] ).Items[ j ] ).Items[ k ] ).pContactor.pContainer.nObj ) and
( TChainPoint( Items[ i - 1 ] ).pContactor.nObj =
TChainPoint( TList( TList( Items[ i ] ).Items[ j ] ).Items[ k ] ).pContactor.nObj ) and
{--- указанная точка - ручка вилки? ---}
( oPoint.pContactor.pContainer.nObj =
TChainPoint( TList( Items[ i ] ).Items[ j - 1 ] ).pContactor.pContainer.nObj ) and
( oPoint.pContactor.nObj =
TChainPoint( TList( Items[ i ] ).Items[ j - 1 ] ).pContactor.nObj ) and
( oPoint.nCont =
TChainPoint( TList( Items[ i ] ).Items[ j - 1 ] ).nCont ) ) then

состоящим из 60 слов (Прямые преобразования и индексы тоже считаю словами, т.к. они несут в себе смысл.
Ищем теперь смысловые объекты:
-nObj
-nCont

Итого 2. Все остальное - это всего-навсего адресация, т.е. вычисление базы, относительно которой определяются смещения для извлечения наших объектов.

Итого, для того, чтобы сделать сравнение типа
if (a1=b1) and (a2=b2) and (a3=b3) and (a4=b4)
ты рисуешь зубодробительную конструкцию, по которой вообще ничего нельзя понять. Отдельный вопрос, как все это будет себя вести при пошаговой отладке.

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

← →
msguns © (2005-09-16 16:57) [54]

Слово смысл заменить на зрительную нагрузку

← →
Игорь Шевченко © (2005-09-16 17:04) [55]

Sandman29 (16.09.05 16:44) [52]

А что "оно" делает ?

← →
Sandman29 (2005-09-16 17:10) [56]

Игорь Шевченко © (16.09.05 17:04) [55]
>А что "оно" делает ?

Для рефакторинга смысл не важен. Избавился от некоторых повторяющихся операций, вот и все.

← →
Игорь Шевченко © (2005-09-16 17:19) [57]

Sandman29 (16.09.05 17:10) [56]

> Для рефакторинга смысл не важен

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

← →
Desdechado © (2005-09-16 17:19) [58]

Sandman29 (16.09.05 16:44) [52]
Можно и так, но это лишние накладные расходы на вызов методов. А если критично время выполнения? Да и для сравнения 2 чисел писать метод? Не гуд, имхо...

msguns © (16.09.05 16:55) [53]
О скорости я речь не вел. Хотя если грамотно построить условие, то она будет выше в моем случае, без переменных. Память - не вопрос.

А вот количество переменных, которые еще не запутывают свои количеством, а помогают понимать смысл операций - это вопрос.
Даже в примере с "зубьями" их 10 штук, а в [51] я указал, что таких сравнений много. Плодить еще 3 десятка переменных, имхо, - только задуривать голову себе и другим. Повторно их использовать с разными смыслами в разных местах - глотать баги.

И здесь не религия, а простой здравый смысл - все хорошо в меру. Если бы значение использовалось несколько раз, еще куда ни шло. А сравнение (oPoint.nCont = TChainPoint( TList( Items[ i ] ).Items[ j - 1 ] ).nCont ), имхо, не намного тяжелее понять, чем (a=b), пытаясь вспомнить, что это за А, и что это за В (и так много раз).

← →
Sandman29 (2005-09-16 17:36) [59]

>Здрасьте, приехали. Рефакторинг - не избавление от повторяющихся операторов, а улучшение существующего кода. То есть, код должен быть понятен даже стороннему человеку.

Согласен. Но на НАЧАЛЬНОЙ стадии рефакторинга смысл не важен.
Если мы имеем a1.b.c=a2.b.c and a3.b.c=a4.b.c, то следует заменить на a1.equals(a2) and a3.equals(a4) независимо от смысла b, с и сравнений в целом.
Естественно, вместо ContactorContainernObjEqualsAndContactorObjEquals следует ввести понятное название метода.

← →
Игорь Шевченко © (2005-09-16 17:40) [60]

Sandman29 (16.09.05 17:36) [59]

> Но на НАЧАЛЬНОЙ стадии рефакторинга смысл не важен

В корне не согласен. Смысл должен быть первой целью и от смысла надо отталкиваться. Весьма вероятно, что код не нужен вообще, какой смысл убирать в нем операции ? :)

← →
Sandman29 (2005-09-16 17:41) [61]

>Можно и так, но это лишние накладные расходы на вызов методов. А если критично время выполнения? Да и для сравнения 2 чисел писать метод? Не гуд, имхо...

Вот как раз из-за времени выполнения - здесь необходимо избавиться от множественных Items[i]. А что касается замены на метод, то основное его назначение - сделать код понятным и удобным для сопровождения. Не дай бог Вам придется изменить структуру записи или вместо TList использовать другой контейнер :)

← →
Sandman29 (2005-09-16 17:46) [62]

Игорь Шевченко © (16.09.05 17:40) [60]

Ну если получится СРАЗУ найти в таком коде смысл - пожалуйста :)
Хотя вот я читал, что рефакторинг делается поэтапно, последовательностью простых типовых операций. Человеческий разум легче (и по времени, и вообще, по результату) решает несколько простых задач, чем одну сложную.
Вы не путаете рефакторинг с полным перепроектированием/переписыванием?

← →
Джо © (2005-09-16 17:49) [63]

[51] Desdechado © (16.09.05 16:27)
> теперь представь, что это фрагмент других подобных обработок,
> которые отличаются только индексом в самом глубоком Items
> (и не только).

Ну вот в этом-то и есть самая главная проблема. Совершенно неоправданное жуткое дублирование кода в классическом варианте. С него и нужно начинать исправлять.

← →
Игорь Шевченко © (2005-09-16 17:58) [64]

Sandman29 (16.09.05 17:46) [62]

Я полагаю, что автору смысл известен ?

Джо © (16.09.05 17:49) [63]
> Совершенно неоправданное жуткое дублирование кода
где ж там дублирование? отличие индекса в самой глубине - это не дублирование, это другой адрес

И вообще, заклевали, но не убедили.
Чтобы сравнить 2 адреса или числа, вовсе не обязательно городить методы, временные переменные и прочее. А то, что строка получения адреса несколько длинна, компилятору не мешает, да и мне тоже, если оно работает годами и не требует изменений.

> [65] Desdechado © (16.09.05 18:22)
> где ж там дублирование? отличие индекса в самой глубине
> - это не дублирование, это другой адрес

Дублирование в том, что, по твоему уверению, есть много повторяющихся кусков, которые отличаются только "индексом где-то в глубине".

> Взгляни на обсуждаемый пост [21] и посчитай, сколько там
> будет переменных, необходимых до провозглашенной красоты
> (чтобы строчка кода была одна).

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

> А сравнение (oPoint.nCont = TChainPoint( TList( Items[ i
> ] ).Items[ j - 1 ] ).nCont ), имхо, не намного тяжелее понять,
> чем (a=b), пытаясь вспомнить, что это за А, и что это за
> В (и так много раз).

Если ты помнишь что такое TList( Items[ i
> ] ).Items[ j - 1 ] ).nCont, то что такое a и чему равно b точно запомнишь.

> Чтобы сравнить 2 адреса или числа, вовсе не обязательно
> городить методы, временные переменные и прочее

Множественные приведения типов и всякие Items-ы тоже.

Джо © (16.09.05 18:24) [66]
в глубине каждого операнда

MeF Dei Corvi © (16.09.05 18:30) [67]
> что такое a и чему равно b точно запомнишь
ключевое слово - и так много раз, много всяких А и В, структура которых одинакова (и это легко запомнить), а вот смысловое значение в каждом случае - свое

> ключевое слово - и так много раз, много всяких А и В, структура
> которых одинакова (и это легко запомнить), а вот смысловое
> значение в каждом случае - свое

А кто сказал, что эти переменные должны быть именно a и b? Можно и какие-нибудь ZubCount, ZubLastCount и пр...
А вообще, имхо такой качество кода зависит от умений и опыта в проектировании.

MeF Dei Corvi © (16.09.05 19:02) [69]
Да дело не в названии. Хотя 40 названий для одного и того же (почти) надо еще уметь придумать. А потом не забыть, что есть что.
И если речь зашла о качестве кода, прошу критерии оценки в студию.

Я считаю, что в угоду весьма сомнительной дополнительной читабельности все остальные критерии качества приносить в жертву не стоит.

Desdechado © (16.09.05 19:11) [71]

> Я считаю, что в угоду весьма сомнительной дополнительной
> читабельности все остальные критерии качества приносить
> в жертву не стоит.

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

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

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

Доработки программ - это отдельный вопрос. Многие мои вещи написаны давно и доработок не требуют (во как :). А некоторые требуют. Но не все же написано, как в том примере. То же был пример "ужастика", которого, кстати, из моих коллег никто не боится. И всем понятен. Чего и вам желаю.

Ценные мысли я впитываю. И то, что здесь я высказываю свое мнение, не должно восприниматься как "никого, кроме себя не слушать". А должно - как моя точка зрения. При этом еще раз повторю при те самые "условия" - ситуации бывают разные. И не надо все под одну гребенку. Единых правил на все случаи жизни не бывает.

Desdechado © (18.09.05 23:01) [74]

> Вот только человек этот - пользователь

Не только пользователь. Еще, как это часто бывает, и программист. И вот для того, чтобы он свои (а в особоенности чужие) программы читал и переводил без словаря, программы должны быть написаны так, чтобы они были понятны. Пользователю действительно неинтересно, как написаны программы, но в мире программирования давно прослеживается тенденция к тому, что если программы написаны непонятно, то работа их тоже не всегда понятна.
Разумеется, я имею в виду не программы уровня "Hello, world", там непонятно написать трудно и места для внесения ошибок немного.

> То же был пример "ужастика", которого, кстати, из моих коллег
> никто не боится. И всем понятен. Чего и вам желаю.

За пожелание, конечно, спасибо, и вам того же.

Но у меня есть один вопрос - с какого раза коллеги поняли, что в этом куске кода происходит ?

> И не надо все под одну гребенку. Единых правил на все случаи
> жизни не бывает.

Безусловно. Код программы, сопровождения которой не требуется или требуется не более, чем одним человеком в течение времени, пока он не успел забыть детали реализации, как угодно. В остальных случаях код следует писать так, чтобы "ужастиков" не появлялось. Писать комментарии, выбирать удачные имена для методов и переменных, организовывать простую и ясную иерархию классов, и т.д.

Ужас! Дожились... Найти похожие ветки