Просьба оценить пример кода

← →
Makhanev A.S. (2003-02-21 00:17) [0]

Интересно, что скажет профессионал, взглянув на приведённый ниже код.

Данная функция возвращает текст песни из ID3v2 тэга MP3-файла.

{gets lyrics text from USLT frame of ID3v2 tag} function GetLyrics(const FileName: string): TStrings; var F: file; Chars: array[1..2]of Char; Str: string; NilCharsCount: Byte; // counter of nil chars in the USLT frame, // after founding of 3 nil chars process (while..do) stops begin Result := TStringList.Create; NilCharsCount := 0; AssignFile(F, FileName); Str := ""; try Reset(F, 1); Seek(F, FilePos(F) + 10); while NilCharsCount <> 3 do begin try BlockRead(F, Chars, 2); except raise Exception.Create("Invalid ""USLT"" frame"); end; if Chars[1] < #32 then begin if (NilCharsCount > 0) and (Trim(Str) <> "") then Result.Add(Trim(Str)); if Pos("USLT", Str) > 0 then begin // "opening" the nil chars count NilCharsCount := 1; Seek(F, FilePos(F) + 9); end; if Chars[2] < #32 then Str := "" else Str := Chars[2]; end else Str := Str + Chars; // if nil chars count is "open" then incrementing the counter in specified cases if (NilCharsCount > 0) and ((Chars[1] = #0) or (Chars[2] = #0)) then Inc(NilCharsCount); end; finally CloseFile(F); end; end;

← →
int64 (2003-02-21 05:07) [1]

Совет №1.
Для таких функций лучше, чтобы результатом был код выполнения, например булевского типа. А TStrings не надо инициировать в теле, получай его по сылке.
Хотя, если хочешь добиться "каллиграфического почерка" кодинга, то его можно шлифовать до бесконечности.

PS/ Сдаётся мне, Игорь Шевченко не принял сиё творение, отсюда и такие просьбы. :)

← →
Юрий Зотов (2003-02-21 10:28) [2]

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

1. Создание TStrringList"а самой функцией в данном случае - очень скверный стиль (чревато утечками памяти, если человек, использующий функцию, недостаточно знаком с ее "устройством"). Ссылку на TStrings надо получать снаружи и НИ В КОЕМ СЛУЧАЕ НЕ ПРОВЕРЯТЬ НА КОРРЕКТНОСТЬ (это будет маскировка ошибок юзера). Если функция написана для использования другими, то такой стиль просто никуда не годится. Если же функция написана только "для себя", то такой стиль еще более-менее приемлем (хотя и самоподстраховка все равно бы не помешала) - но тогда это говорит о недостаточном уровне "системного" мышления программиста и о том, что он не вполне готов работать "в команде".

2. Если уж используется try-finally, то try должно стоять ПОСЛЕ Reset (поскольку файл открывает именно Reset, а не AssifnFile). Боюсь, это может свидетельствовать о неполном понимании или работы с файлами, или работы с защищенными блоками.

Вот такое вот IMHO.

← →
Andrey (2003-02-21 10:44) [3]

Как сказал однажды Anatoly Podgoretsky на просьбу оценить код: 47 строк.

:)

← →
Danilka (2003-02-21 10:44) [4]

int64 © (21.02.03 05:07)
можно отдавать стринг...
и набирать в стринг а не стринглист... #13#10 никто не отменил..

а снаружи делать так:
StringList.Text:=GetLyrics("xxx.mp3");

← →
Ketmar (2003-02-21 10:56) [5]

всё умное уже сказали. добавлю, что я бы использовал файловые потоки вместо file. всё равно classes тащишь.

Satanas Nobiscum! 21-Feb-XXXVIII A.S.

← →
Andrey (2003-02-21 10:56) [6]

И еще:
BlockRead(F, Chars, 2);
по 2 байта читать нехорошо, операционка конечно кеширует чтение/запись, но пока она проверит все входные параметры пройдет много времени, уж лучше читать блоками хоть по 4К.

← →
REA (2003-02-21 15:58) [7]

А оно не зациклится на произвольном файле?

← →
Mike_Goblin (2003-02-21 16:18) [8]

1. См Юрий Зотов. Утечка памяти будет, если вызывающий забудет вызвать деструктор для объекта-результата, а он это забудет :)))
2. Я бы разбил ее на кусочки поменьше, так как анализировать Ваш код сложно - большой код метода - прямой путь к ошибкам.
Вы
- открываете файл
- читаете из него в цикле
- анализируете содержимое
логично написать так, чтобы каждый из кусочков задачи четко просматривался и контролировался

Как минимум стоит вынести блок
if Chars[1] < #32 then
....
else
...
В подметод с понятным именем типа AnalyzeChars
PS в суть кода глубоко не вникал

← →
Suntechnic (2003-02-21 16:48) [9]

Уже почти всё сказали, добавлю и я свои пять копеек. Тот memory leak, о котором так долго говорили мастера, не то что может произойти, а произойдёт если вдруг ф-ция BlockRead fails.

Запомни одно правило: где память распределяется, в том же scope, как правило, она и должна удаляться.

← →
Makhanev A.S. (2003-02-21 23:38) [10]

> int64 © (21.02.03 05:07)

Совет №1 принят.
Что касается Игоря Шевченко, то я и не собирался ему что-либо слать. Мал я ещё (1 год с хвостиком в Delphi)...

каллиграфический почерк мне не нужно, мне нужен правильный почерк.

> Юрий Зотов © (21.02.03 10:28)

Честно говоря, слов типа "вроде нормальный" никак не ожидал...
Данный код считаю достаточно корявым, просто опыта малова-то было... А правильный код без опыта, ИМХО, - вещь редкая.
Что касается 1-го замечания, то всё ясно и понятно, полностью согласен.
Что касается 2-го замечания, то я всегда писал try после assignfile и перед reset, rewrite и append. Просто как-то то ли в книжке, то ли в справке встретил такой пример и придерживался его.
По идее - ваше замечание №2 верно: если reset не сработает, зачем делать closefile, ведь он и так закрыт будет... Я правильно Вас понял?

Спасибо Вам за замечания, берём на вооружение.

> Andrey © (21.02.03 10:44)

хороший ответ:)))

> Danilka © (21.02.03 10:44)

Тоже вариант, но код написан мною для себя же, а я не люблю лишних преобразований...

> Ketmar © (21.02.03 10:56)

Если файловый поток будет быстрей работать, то - вариант. Вначале так и думал сделать, но данный метод мне больше понравился.

> Andrey © (21.02.03 10:56)

Читать такими блоками (4К) нереально, читается только часть заголовка файла, это порядка 40-600 байт. Да к тому же в строке куча спец. символов, по 2 байта, ИМХО, работать много проще, т.к. мне постоянно нужны 2 соседних байта.

> REA © (21.02.03 15:58)

Нет, сгенерируется EInOutError при очередной попытке BlockRead, выведется соответствующее сообщение, а затем CloseFile. (я не стал выкладывать проверку, не этом суть).

> Mike_Goblin © (21.02.03 16:18)

1.С утечкой памяти понятно.
2.Попробовал разбить на подметоды... не очень понравилось, как-то мне в таком "узле" проще. Когда закончу разбиение, окончательно пойму, стоит оно того, или нет.

> Suntechnic © (21.02.03 16:48)

Утечка произойдёт, понятно.
Насчёт правила - спасибо, очень ценное замечание, давно к нему шёл, но вот всё никак не мог "дойти".

P.S.: всем огромное спасибо за советы. Буду стараться больше читать и думать над кодом. Честно говоря, сам оцениваю его ниже, чем Вы.... может Вы из вежливости... так ... мягко с мною обошлись?

← →
Юрий Зотов (2003-02-22 00:12) [11]

> Makhanev A.S. © (21.02.03 23:38)
> Я правильно Вас понял?

Совершенно правильно. Вот более наглядный пример на ту же тему.

try
Obj := TMyObject.Create;
...
finally
Obj.Free
end;

Предположим, по каким-то причинам в конструкторе возникло исключение. В этом случае только что созданный объект будет автоматически уничтожен в самом же конструкторе (компилятор генерит для этого специальный скрытый код) и в результате переменная Obj не будет содержать верной ссылки на объект. А затем, после показа сообщения об ошибке, будет выполнена секция finally - и мы получаем ВТОРОЕ исключение!!!

Сравните с таким вариантом:

Obj := TMyObject.Create;
try
...
finally
Obj.Free
end;

Или с таким:

with TMyObject.Create do
try
...
finally
Free
end;

Здесь все корректно - если в конструкторе возникает ошибка, объект уничтожается сам по себе, мы получаем сообщение, а более НИЧЕГО не происходит.

Общий вывод - ту строку, в которой СОЗДАЕТСЯ или ЗАХВАТЫВАЕТСЯ защищаемый ресурс, НЕ НУЖНО включать в защищенный блок. Потому что, если при создании/захвате возникла ошибка, то ничего создано/захвачено не будет. Значит, и освобождать просто нечего. Это относится ко всем видам ресурсов, включая и файлы.

← →
Makhanev A.S. (2003-02-22 01:07) [12]

← →
AZ (2003-02-22 04:54) [13]

Спасибо всем и от меня.
Почаще бы делался подобный "разбор полетов".

← →
Ketmar (2003-02-22 12:59) [14]

>Makhanev A.S. © (21.02.03 23:38)
не будет файловый поток быстрее. потому что оба метода через API работают. поток удобнее.

Satanas Nobiscum! 22-Feb-XXXVIII A.S.

Просьба оценить пример кода Найти похожие ветки