Главная страница
    Top.Mail.Ru    Яндекс.Метрика
Форум: "Потрепаться";
Текущий архив: 2003.03.10;
Скачать: [xml.tar.bz2];

Вниз

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

 
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]


> Юрий Зотов © (22.02.03 00: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.



Страницы: 1 вся ветка

Форум: "Потрепаться";
Текущий архив: 2003.03.10;
Скачать: [xml.tar.bz2];

Наверх





Память: 0.61 MB
Время: 0.044 c
3-29724
Solontsov
2003-02-18 21:21
2003.03.10
Отфильтровать Table по двум полям


7-30146
q777
2003-01-11 16:57
2003.03.10
DefineDosDevice


7-30147
cc43294
2003-01-11 17:02
2003.03.10
Modem


7-30144
Dor
2003-01-08 19:37
2003.03.10
Как узнать сколько памяти занято и сколько свободно???Очень надо


7-30143
ychnic
2003-01-10 15:25
2003.03.10
Компонент





Afrikaans Albanian Arabic Armenian Azerbaijani Basque Belarusian Bulgarian Catalan Chinese (Simplified) Chinese (Traditional) Croatian Czech Danish Dutch English Estonian Filipino Finnish French
Galician Georgian German Greek Haitian Creole Hebrew Hindi Hungarian Icelandic Indonesian Irish Italian Japanese Korean Latvian Lithuanian Macedonian Malay Maltese Norwegian
Persian Polish Portuguese Romanian Russian Serbian Slovak Slovenian Spanish Swahili Swedish Thai Turkish Ukrainian Urdu Vietnamese Welsh Yiddish Bengali Bosnian
Cebuano Esperanto Gujarati Hausa Hmong Igbo Javanese Kannada Khmer Lao Latin Maori Marathi Mongolian Nepali Punjabi Somali Tamil Telugu Yoruba
Zulu
Английский Французский Немецкий Итальянский Португальский Русский Испанский