A>>Оставь как есть. Только испортишь. Константы особенно понравились: 34359738368. Чел знает толк в таких вещах.
J>если бы дело в драйверах или подобной хрени. Это код выгрузки документа в Excel
Не важно что это, потому что это просто красиво. Утилитарист какой-нибудь написал бы $800000000 и все испортил
Re: Я думал, я так себе программист, но меня посадили рефакт
if ((OutStyle and 34359738368) <> 0) and
((FormNum = 7) or (FormNum = 18)) and
(((OutStyle and 70368744177664) = 0) or
(((OutStyle and 70368744177664) <> 0) and (OneAct))) then
NRow := BuildItogTwoPriceLevels(OO, true, false, ACell, -1, NRow, FormNum, OutStyle, nil, {[]}DocOptions, false)
if Trim(FName)='Пустой идентификатор'then Result := aNone
else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
else if Trim(FName)='Номер позиции по смете'then Result := aNpp
else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
// далее 900 таких же строк
// классы TSmeta/TCustomObjSmeta/TCennik - конкретные классы, все наследники базового класса, у которого как раз объявлено свойство DocSignaturesfunction GetSignByID(ToAct: boolean; ASmeta: TSmeta; ObjSm: TCustomObjSmeta; ACennik: TCennik; AResSummary: TResSummary;
AProjectSmeta: TProjectSmeta; AWV: TWorksVolumes; ADS: TDocSummary; TC: TTranspCalc; CaptionSign: boolean; IDS: integer; IDA: integer): string;
begin
....
if ObjSm<>nil then SSS := TGsDocSignatures(ObjSm.DocSignatures);
if ACennik<>nil then SSS := TGsDocSignatures(ACennik.DocSignatures);
if AResSummary<>nil then SSS := TGsDocSignatures(AResSummary.DocSignatures);
if AProjectSmeta<>nil then SSS := TGsDocSignatures(AProjectSmeta.DocSignatures);
if AWV<>nil then SSS := TGsDocSignatures(AWV.DocSignatures);
if ADS<>nil then SSS := TGsDocSignatures(ADS.DocSignatures);
if TC<>nil then SSS := TGsDocSignatures(TC.DocSignatures);
...
end;
if
(NeedResourceType(ARow) or ((RootType = rrtDeleted) and (exopDelRes in DocOptions)) and
((FormNum = 20) or (FormNum = 21) or (FormNum = 13))) or
(TSmetaResourceTypeEx(ResourceType) = smrtEquip) or
((FormNum <> 20) and (FormNum <> 21) and (FormNum <> 13))
then
if (((FormNum = 20) or (FormNum = 21) or (FormNum = 13)) and// !
(((ReportString[sdmQtTot] = '') and
((OutStyle and 2097152) <> 0)) or
(ReportString[sdmQtTot] <> ''))) or
((FormNum <> 20) and (FormNum <> 21) and (FormNum <> 13))
then// !begin
Re: Я думал, я так себе программист, но меня посадили рефакторить код одного кол
J>if Trim(FName)='Пустой идентификатор'then Result := aNone
J> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J> // далее 900 таких же строк
J>
Это код коллеги? Или это код десятка предыдущих коллег, первый из которых работал 10-20 лет назад?
И я подозреваю, что началось всё с небольшой софтины, в которой было не 900, а 4-5 строк. "Пока что сделаю быстро так, а то времени нет возиться со всякими id".
А потом строки наращивали. Почему наращивали? Чтобы не рефакторить. А то вдруг сломается. Работает — не трогаем.
Re[3]: Я думал, я так себе программист, но меня посадили рефакт
Здравствуйте, Jack128, Вы писали:
>если бы дело в драйверах или подобной хрени.
В драйверах суровые бородатые дядьки за такое сразу роняют барабанный принтер на руки автору.
... << RSDN@Home 1.3.110 alpha 5 rev. 62>>
Re[2]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, Sharov, Вы писали:
S>Здравствуйте, Jack128, Вы писали:
J>>
J>>if Trim(FName)='Пустой идентификатор'then Result := aNone
J>> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J>> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J>> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J>> // далее 900 таких же строк
J>>
S>Тут что не так, за исключением того, что Trim, возможно, будет вызываться 900 раз?
Я бы сделал примерно так:
type
TSomeType = (aNone, aActivePos, aNpp, aNppImp);
TStrToSomeTypeRec = record
Str: string;
SomeType: TSomeType;
end;
const
StrToTypeArray: array[0..3] of TStrToSomeTypeRec = (
(Str: 'Пустой идентификатор'; SomeType: aNone),
(Str: 'Учитываемая позиция'; SomeType: aActivePos),
(Str: 'Номер позиции по смете'; SomeType: aNpp),
(Str: 'Номер позиции по порядку (в актах выполненных работ)'; SomeType: aNppImp));
function StrToSomeType(Str: string): TSomeType;
var
Index: Integer;
begin
Str := Trim(Str);
for Index := Low(StrToTypeArray) to High(StrToTypeArray) do
if Str = StrToTypeArray[Index].Str then
Exit(StrToTypeArray[Index].SomeType);
raise Exception.Create('String not found');
end;
Re: Я думал, я так себе программист, но меня посадили рефакторить код одного кол
Здравствуйте, varenikAA, Вы писали:
AA>Это какая версия? AA>В первом случае непонятно, это метод класса или функция? AA>Во втором, да можно было словарик прикрутить.
Со словариком стало бы только хуже, IMHO, пришлось бы вместо одного в двух местах править при изменении структуры файла Excel, с которым этот код работает.
А так как есть IMHO удобнее — вся бизнес-логика сконцентрирована в одном месте.
Вот длинным константам и номерам форм у меня как-то с трудом находится оправдание. Должна же быть тут какая-то логика?
Может он не знал что там можно шестнадцатеричные числа писать? Вряд ли.
Или может раньше нельзя было (из-за какого-нибудь бага в компиляторе например)?
Номера форм, возможно в организации все формы (бумажные) имеют номер.
Типа, "форма номер 61Ф". Тогда в номерах появляется смысл.
В общем, я бы это древнее зло не трогал, во избежание
Я недавно видел свеженаписанный код. В нём КАЖДАЯ строка была с комментарием на русском. Но! Частенько содержимое комментария не совпадало с действием в коде.
Ну и апофеозом была функция с 18 вложенными if. 18 Карл! Восемнадцать! 0x12!
Re[2]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, Sharov, Вы писали:
S>Здравствуйте, Jack128, Вы писали:
J>>
J>>if Trim(FName)='Пустой идентификатор'then Result := aNone
J>> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J>> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J>> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J>> // далее 900 таких же строк
J>>
S>Тут что не так, за исключением того, что Trim, возможно, будет вызываться 900 раз?
дизайн приложения очень фиговый. это означит что эти строки не только здесь фигрурируют но и где-то еще, и если кто-нибудь попросит немного поменять текст — замудохаешься его менять в двух (я подозреваю что при описанном стиле кодирования то и в трех и в четырх и т.д.) местах.
т.е. вместо этих магических строковых констант должен быть енум, типа EMPTY_ID("Пустой идентификатор"), ...
c enumами можно swtich/case использовать. правда я не знаю есть ли енумы в дельфи или что тут за язык
Re[2]: Я думал, я так себе программист, но меня посадили реф
Здравствуйте, andyp, Вы писали:
A>Здравствуйте, Jack128, Вы писали:
J>>Из прекрасного:
A>Оставь как есть. Только испортишь. Константы особенно понравились: 34359738368. Чел знает толк в таких вещах.
если бы дело в драйверах или подобной хрени. Это код выгрузки документа в Excel
Re[4]: Я думал, я так себе программист, но меня посадили рефакт
Здравствуйте, andyp, Вы писали:
A>Не важно что это, потому что это просто красиво. Утилитарист какой-нибудь написал бы $800000000 и все испортил
Ох, ты не представляешь, как испортил бы, особенно, если учесть, что в Delphi (а код на нем) — еще есть и специальный тип и поддержка языка для работы с битовами масками.
Re: Я думал, я так себе программист, но меня посадили рефакторить код одного кол
Здравствуйте, Jack128, Вы писали:
J>Из
а кто это писал такое, можешь описать человека? не фио, а так, типа черты характера, возраст, семейное положение.
Re[5]: Я думал, я так себе программист, но меня посадили рефакт
Здравствуйте, Jack128, Вы писали:
J>Ох, ты не представляешь, как испортил бы, особенно, если учесть, что в Delphi (а код на нем) — еще есть и специальный тип и поддержка языка для работы с битовами масками.
Вот видишь! Получилась бы новомодная бездуховщина. А так — тепло и лампово, как деды завещали!
... << RSDN@Home 1.3.110 alpha 5 rev. 62>>
Re: Я думал, я так себе программист, но меня посадили рефакторить код одного кол
Здравствуйте, зиг, Вы писали:
зиг>Здравствуйте, Jack128, Вы писали:
J>>Из зиг>а кто это писал такое, можешь описать человека? не фио, а так, типа черты характера, возраст, семейное положение.
У нас есть такой на конторе товарищ.
Кстати,тоже пишет на делфях.
Самой конторе 25+ лет, софту еще больше, миллионы строк кода.
Челу 65+ лет, живет в новой зеландии, наверное уже дедушка. Программит больше для души, но когда чтото фиксит, то падает вокруг все подряд. Держат на конторе в том числе и по причине выслуги лет.
"Ну дела, — подумал Лось, —
Не хотелось.
А пришлось". (c)
Re[2]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, bnk, Вы писали:
bnk>Может он не знал что там можно шестнадцатеричные числа писать? Вряд ли. bnk>Или может раньше нельзя было (из-за какого-нибудь бага в компиляторе например)?
Даже если нельзя писать числа в хексе, такую бороду всегда можно спрятать за константу с красивым названием.
Re[2]: Я думал, я так себе программист, но меня посадили рефакт
J>if Trim(FName)='Пустой идентификатор'then Result := aNone
J> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J> // далее 900 таких же строк
J>
Тут что не так, за исключением того, что Trim, возможно, будет вызываться 900 раз?
Кодом людям нужно помогать!
Re[2]: Я думал, я так себе программист, но меня посадили рефакторить код одного
J>>if Trim(FName)='Пустой идентификатор'then Result := aNone
J>> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J>> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J>> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J>> // далее 900 таких же строк
J>>
LK>И я подозреваю, что началось всё с небольшой софтины, в которой было не 900, а 4-5 строк. "Пока что сделаю быстро так, а то времени нет возиться со всякими id".
Зачем id, потом sed натравливаешь, и всё работает.
Может у коллеги даже собственный софт есть по модификации своего кода.
Re: Я думал, я так себе программист, но меня посадили рефакторить код одного кол
Здравствуйте, Sharov, Вы писали:
S>Здравствуйте, Jack128, Вы писали:
J>>
J>>if Trim(FName)='Пустой идентификатор'then Result := aNone
J>> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J>> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J>> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J>> // далее 900 таких же строк
J>>
S>Тут что не так, за исключением того, что Trim, возможно, будет вызываться 900 раз?
Если таких строк уже 900, то они не должны быть в самом коде. Вынести в какой нибудь внешний текстовый файл-конфиг и при старте зачитывать в дерево для быстрого поиска.
Re[3]: Я думал, я так себе программист, но меня посадили рефакторить код одного
J>>>if Trim(FName)='Пустой идентификатор'then Result := aNone
J>>> else if Trim(FName)='Учитываемая позиция'then Result := aActivePos
J>>> else if Trim(FName)='Номер позиции по смете'then Result := aNpp
J>>> else if Trim(FName)='Номер позиции по порядку (в актах выполненных работ)'then Result := aNppImp
J>>> // далее 900 таких же строк
J>>>
S>>Тут что не так, за исключением того, что Trim, возможно, будет вызываться 900 раз?
D>Если таких строк уже 900, то они не должны быть в самом коде. Вынести в какой нибудь внешний текстовый файл-конфиг и при старте зачитывать в дерево для быстрого поиска.
Ужас какой. Зачем!? Это же работа с Excel, логику там наверняка правят не по разу.
Предлагаешь теперь десять файлов править вместо одного, да еще и фреймворк писать для чтения конфига?!
Re[3]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, Socrat, Вы писали:
S>Здравствуйте, Sharov, Вы писали:
S>>Тут что не так, за исключением того, что Trim, возможно, будет вызываться 900 раз?
S>Я тоже особого криминала не вижу. Хотя, насколько я помню, в паскале case работает со строками.
не работает. только с ordinal types: char, byte, integer, dword etc.
ну и вызывать Trim() 900 раз....
неужели так сложно было
s := Trim(FName);
if s='Пустой идентификатор' then Result := aNone
else if s='Учитываемая позиция' then Result := aActivePos
else if s='Номер позиции по смете' then Result := aNpp
else if s='Номер позиции по порядку (в актах выполненных работ)' then Result := aNppImp
Re[3]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, зиг, Вы писали:
зиг>дизайн приложения очень фиговый. это означит что эти строки не только здесь фигрурируют но и где-то еще
Не факт. Представь, что тебе приходит информация от сторонней организации в виде xml-файла, в котором каждое поле имеет свое название. Причем, организаций может быть не одна. И формат может меняться раз в несколько лет. А этот код переводит название этих полей в собственный enum.
Re[3]: Я думал, я так себе программист, но меня посадили реф
Здравствуйте, зиг, Вы писали:
зиг>дизайн приложения очень фиговый. это означит что эти строки не только здесь фигрурируют но и где-то еще, и если кто-нибудь попросит немного поменять текст — замудохаешься его менять в двух (я подозреваю что при описанном стиле кодирования то и в трех и в четырх и т.д.) местах. зиг>т.е. вместо этих магических строковых констант должен быть енум, типа EMPTY_ID("Пустой идентификатор"), ... зиг>c enumами можно swtich/case использовать. правда я не знаю есть ли енумы в дельфи или что тут за язык
Сомневаюсь что так было бы лучше. Так еще и идентификаторы эти поддерживать пришлось бы.
Если предположить, что этот код добывает данные из файла Excel (или нескольких), который некая сердобольная тетенька верстает, как ей заблагорассудится,
или парсит скачанный откуда-то файл, не предназначенный для машинного потребления (некий отчет-презентация например), то все становится не так однозначно.
Поддержка повторного использования, или какая-то оптимизация тут IMHO абсолютно не нужна и даже будет вредна.
А вот трим, да, похоже на копи-пасту. Но может там где-то важны пробелы в начале-конце, я х.з.
Здравствуйте, bnk, Вы писали:
bnk>Здравствуйте, зиг, Вы писали:
зиг>>дизайн приложения очень фиговый. это означит что эти строки не только здесь фигрурируют но и где-то еще, и если кто-нибудь попросит немного поменять текст — замудохаешься его менять в двух (я подозреваю что при описанном стиле кодирования то и в трех и в четырх и т.д.) местах. зиг>>т.е. вместо этих магических строковых констант должен быть енум, типа EMPTY_ID("Пустой идентификатор"), ... зиг>>c enumами можно swtich/case использовать. правда я не знаю есть ли енумы в дельфи или что тут за язык
bnk>Сомневаюсь что так было бы лучше. Так еще и идентификаторы эти поддерживать пришлось бы.
что значит пришлось? их не надо поддерживвать, их один раз написал и потом везде используешь, и иначе оно компилится не будет. или я не понимаю о чем ты?
bnk>Если предположить, что этот код добывает данные из файла Excel (или нескольких), который некая сердобольная тетенька верстает, как ей заблагорассудится, bnk>или парсит скачанный откуда-то файл, не предназначенный для машинного потребления (некий отчет-презентация например), то все становится не так однозначно.
ну это понятно, нужен контекст. а так код выдран из контекста и не понятно. мне поначалу показалось что это какая-то сложная аппликуха со сложным UI на котором вот эти все кнопки, окна, выпадающие списки и т.д. и чувак по надписи на нажатой кнопке решает что делать дальше
bnk>Поддержка повторного использования, или какая-то оптимизация тут IMHO абсолютно не нужна и даже будет вредна.
ок, моё ИМХО код очевидно ужасен этими строковыми константами на основе которых что-то там хитрое определяется.
Re[4]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, Socrat, Вы писали:
S>Здравствуйте, зиг, Вы писали:
зиг>>дизайн приложения очень фиговый. это означит что эти строки не только здесь фигрурируют но и где-то еще
S>Не факт. Представь, что тебе приходит информация от сторонней организации в виде xml-файла, в котором каждое поле имеет свое название. Причем, организаций может быть не одна. И формат может меняться раз в несколько лет. А этот код переводит название этих полей в собственный enum.
тогда я бы все равно сделала по-другому. у меня все равно был бы енум, и чтоб получить его инстанс я бы вызывала MyEnum.valueOf(stringReceivedFromXML) а потом был бы свитч по енуму
Re[5]: Я думал, я так себе программист, но меня посадили реф
Здравствуйте, зиг, Вы писали:
bnk>>Сомневаюсь что так было бы лучше. Так еще и идентификаторы эти поддерживать пришлось бы. зиг>что значит пришлось? их не надо поддерживать, их один раз написал и потом везде используешь, и иначе оно компилится не будет. или я не понимаю о чем ты?
Непонятно, зачем лишняя сущность (ID)? Например, тот же код на javascript [для уменьшения синтаксического оверхеда (c)], зачем тут какие-то ID?
Подразумевается, что идентификаторы используются ровно в одном месте (здесь), для "адресации" колонки в Excel.
Result = {
'Пустой идентификатор': aNone,
'Учитываемая позиция': aActivePos,
'Номер позиции по смете': aNpp,
'Номер позиции по порядку (в актах выполненных работ)': aNppImp
}[FName.trim()];
Кстати идентификатор прикольный раньше не заметил: BuildItogTwoPriceLevels. Похоже код реально бронтозавр писал, хватит его оправдывать
Здравствуйте, зиг, Вы писали:
зиг>тогда я бы все равно сделала по-другому. у меня все равно был бы енум, и чтоб получить его инстанс я бы вызывала MyEnum.valueOf(stringReceivedFromXML) а потом был бы свитч по енуму
Спорное решение. Код выше прост и очевиден. А это немаловажное преимущество.