Я недавно видел свеженаписанный код. В нём КАЖДАЯ строка была с комментарием на русском. Но! Частенько содержимое комментария не совпадало с действием в коде.
Ну и апофеозом была функция с 18 вложенными if. 18 Карл! Восемнадцать! 0x12!
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[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[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]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, 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[4]: Я думал, я так себе программист, но меня посадили рефакторить код одного
Здравствуйте, зиг, Вы писали:
зиг>дизайн приложения очень фиговый. это означит что эти строки не только здесь фигрурируют но и где-то еще
Не факт. Представь, что тебе приходит информация от сторонней организации в виде 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) а потом был бы свитч по енуму
Спорное решение. Код выше прост и очевиден. А это немаловажное преимущество.
