Оценка качества кода

← →
capkoh © (2008-01-28 15:20) [40]

{*******************************************************} { } { TAnimationTimer class unit } { } { This class is used to define a timer, which } { runs through the number of time intervals } { divisible by TimeStep [ms]. At the end of } { each interval, TimerProc() will be called. } { } { Last modified: 27 January 2008, capkoh } { } {*******************************************************} unit AnimationTimer; interface uses Windows; { --- Properties --- ------------------ To use this timer you need to specify TimeStep, IntervalsCount and it"s duration. TimeStep is the duration of one frame -- this is the minimum time interval. The default value is 40 ms (i.e. 25 FPS). IntervalsCount is the total count of frames in animation. To set IntervalsCount, call SetIntervalsCount(). If an error occured then it will return False. Duration of interval "i" is calculated as: Duration [ms] = TimeStep [ms] * Intervals[i] If you specify zero or negative duration of the interval, then it will be set to one. LoopTimer property valid only when timer started. It allows you to determine whether timer looped. At the end of each interval (and immediatelly after timer start) user-defined TimerProc() will be called if it"s set. The parameter passed to TimerProc() is the zero-based index of currently running interval. ElapsedIntervalIndex in different situations (IntervalsCount = 5): ------------------------------------------- Not Looped | 0 1 2 3 4 5 Looped | 0 1 2 3 4 0 1 2 3 4 0 1 ... -------------------------^----------------- If IntervalsCount = ElapsedIntervalIndex (timer not looped) then last interval has been elapsed and timer stopped (see "5" above). --- Methods ------ ------------------ procedure Start(const Reset, Loop : boolean); Starts timer with parameters provided. If Reset = True, then timer starts from first interval, if False, then continues from current one. procedure Stop(); Stops timer. function SetIntervalsCount(const Count : integer) : boolean; Sets IntervalsCount. Returns False if an error occured. } type PIntervalsList = ^TIntervalsList; TIntervalsList = array [0..0] of integer; type TAnimationTimerProc = procedure (const ElapsedIntervalIndex : integer); type TAnimationTimer = class private fTimerWnd : hWnd; fTimerID : cardinal; fIntervalsCount : integer; fCurrentInterval : integer; fElapsed : integer; fTimerSet : boolean; fLoopTimer : boolean; fTimeStep : integer; fTimerProc : TAnimationTimerProc; pIntervals : PIntervalsList; function GetInterval(Index : integer) : integer; procedure SetInterval(Index : integer; const Duration : integer); public constructor Create(const hWnd : HWND); destructor Destroy(); override; procedure Start(const Reset, Loop : boolean); procedure Stop(); property Intervals[Index : integer] : integer read GetInterval write SetInterval; property IntervalsCount : integer read fIntervalsCount; function SetIntervalsCount(const Count : integer) : boolean; property CurrentInterval : integer read fCurrentInterval; property LoopTimer : boolean read fLoopTimer; property TimeStep : integer read fTimeStep write fTimeStep; property OnTimer : TAnimationTimerProc read fTimerProc write fTimerProc; end; implementation { ------------------------------------------------------------------------ } { --- Common callback function for all TAnimationTimer class instances --- } { ------------------------------------------------------------------------ } procedure CallbackTimerProc(hwnd : HWND; uMsg : UINT; idEvent : UINT; dwTime : DWORD); stdcall; var TargetTimer : TAnimationTimer; CurrentDuration : integer; begin // Cast timer from the idEvent TargetTimer := TAnimationTimer(idEvent); // Unsafe? inc(TargetTimer.fElapsed); // Duration of currently running interval CurrentDuration := TargetTimer.pIntervals^[TargetTimer.fCurrentInterval]; // Avoid infinite counting and null duration problem if TargetTimer.fElapsed > CurrentDuration then TargetTimer.fElapsed := CurrentDuration; // Current interval finished if TargetTimer.fElapsed = CurrentDuration then begin inc(TargetTimer.fCurrentInterval); // What to do when the last interval is finished? if (TargetTimer.fCurrentInterval = TargetTimer.IntervalsCount) then if TargetTimer.fLoopTimer then TargetTimer.fCurrentInterval := 0 else KillTimer(TargetTimer.fTimerWnd, TargetTimer.fTimerID); // Call user proc if assigned if Assigned(TargetTimer.fTimerProc) then TargetTimer.fTimerProc(TargetTimer.fCurrentInterval); TargetTimer.fElapsed := 0; end; end; { TAnimationTimer class implementation } constructor TAnimationTimer.Create(const hWnd : HWND); begin fTimerWnd := hWnd; fTimerID := cardinal(Self); // Unsafe? fTimeStep := 40; // 25 FPS (default) fCurrentInterval := 0; fIntervalsCount := 0; fTimerSet := False; pIntervals := nil; end; destructor TAnimationTimer.Destroy(); begin Stop(); // Kill timer // Free memory if it"s allocated if pIntervals <> nil then HeapFree(GetProcessHeap(), 0, pIntervals); inherited; end; function TAnimationTimer.SetIntervalsCount(const Count : integer) : boolean; var MemorySize : cardinal; hHeap : cardinal; begin hHeap := GetProcessHeap(); Result := hHeap <> 0; // Heap error if not Result then Exit; // Free present memory if pIntervals <> nil then begin Result := HeapFree(hHeap, 0, pIntervals); pIntervals := nil; fIntervalsCount := 0; fCurrentInterval := 0; end; // Incorrect input value if Count < 0 then Result := False; if Result and (Count > 0) then begin // Allocate new memory MemorySize := sizeof(integer) * Count; pIntervals := HeapAlloc(hHeap, 0, MemorySize); { HEAP_ZERO_MEMORY } // Silent error... if pIntervals <> nil then begin ZeroMemory(pIntervals, MemorySize); fIntervalsCount := Count; end else Result := False; end; end; (...)

Уже не влезает. И немного съежает положение строк.
Как вы думаете, такие проверки в методе SetIntervalsCount() – это фанатизм или нет?
Я не стал делать из этого property, потому что непонятно, как вернуть ошибку.

← →
capkoh © (2008-01-28 15:21) [41]

Правда ли, что такое:

property TimeStep : integer read fTimeStep write fTimeStep;

хуже, чем просто такое:

TimeStep : integer;

?

Надеюсь, что стало лучше?

← →
Игорь Шевченко © (2008-01-28 15:45) [42]

> Правда ли, что такое:
>
> property TimeStep : integer read fTimeStep write fTimeStep;
>
>
> хуже, чем просто такое:
>
> TimeStep : integer;

Нет, не хуже.

Просьба будет - комментарии на русский язык переведи.

← →
Loginov Dmitry © (2008-01-28 21:41) [43]

> Как вы думаете, такие проверки в методе SetIntervalsCount()
> – это фанатизм или нет?

Это шизофрения. Зачем столько городить, если можно было обойтись 3-мя строчками.

ЗЫ: вариант с GetMem/FreeMem (или ReallocMem) был бы лучше

← →
capkoh © (2008-01-29 14:11) [44]

> ЗЫ: вариант с GetMem/FreeMem (или ReallocMem) был бы лучше

А как же контроль ошибки выделения памяти? Положиться на то, что вызывающий код может быть будет заключен в try...except? Мне кажется, что это не очень-то надёжно.

Или выделение памяти уже никто не контролирует?

← →
Loginov Dmitry © (2008-01-29 16:59) [45]

>
> А как же контроль ошибки выделения памяти? Положиться на
> то, что вызывающий код может быть будет заключен в try..
> .except? Мне кажется, что это не очень-то надёжно.

если вся программа состоит только из твоих библиотек, то ради бога, контроллируй. Действительно, если массив достигнет 500 млн элементов, то можно словить Out of Memory.

Если же используются стандартные библиотеки, в которых контроля нигде нет, то смысл от твоих проверок скорее всего потеряется.

Согласен. Но я всё-таки стремлюсь всё предусмотреть в своих программах.
А поскольку пишу в последнее время исключительно с использованием WinAPI,
то это, надеюсь, мне удаётся.

Перевод комментариев:
{*******************************************************} { } { TAnimationTimer } { } { Класс используется для создания таймера, } { который последовательно отсчитывает } { некоторое количество интервалов времени. } { Каждый интервал времени должен делиться } { на величину TimeStep [мс]. По окончании } { каждого интервала вызывается функция } { TimerProc(), которая должна быть задана } { пользователем. } { } { Последнее изменение: 27 января 2007, capkoh } { } {*******************************************************} { --- Свойства ----- ------------------ Для того, чтобы начать использовать таймер нужно определить величину TimeStep, количество интервалов IntervalsCount и их продолжительность. TimeStep -- продолжительность одного кадра -- минимальный интервал времени. Значение по умолчанию -- 40 мс (25 кадров/с). IntervalsCount -- общее число изображений в анимации. Для того, чтобы установить IntervalsCount, необходимо вызвать функцию SetIntervalsCount(). Если в процессе установки нового значения возникнут какие-то ошибки, то функция вернёт False. Продолжительность интервала "i" можно определить по формуле: Duration [ms] = TimeStep [ms] * Intervals[i] При попытке установить продолжительность интервала равной нулю, она будет изменена на единицу, поскольку в нулевой продолжительности кадра нет смысла. Свойство LoopTimer верно только после запуска таймера. Оно показывает зациклен таймер или нет. В конце каждого интервала (и сразу после запуска таймера) вызывается пользовательская функция TimerProc(), если она определена. Параметр, передаваемый в функцию TimerProc() -- это номер текущего интервала, отсчитываемый от нуля. ElapsedIntervalIndex в различных ситуациях (IntervalsCount = 5): ---------------------------------------------------- Таймер НЕ зациклен | 0 1 2 3 4 5 Таймер зациклен | 0 1 2 3 4 0 1 2 3 4 0 1 ... ---------------------------------^------------------ Если IntervalsCount = ElapsedIntervalIndex (и таймер НЕ зациклен), то это значит, что закончился последний интервал и таймер остановился. Это соответствует "5" интервалу выше. --- Методы ------- ------------------ procedure Start(const Reset, Loop : boolean); Запускает таймер с указанными параметрами. Если Reset = True, тогда таймер сбрасывается и начинает отсчёт с первого интервала. В противном случае, таймер продолжает счёт с текущего интервала. procedure Stop(); Остатавливает таймер. Текущий интервал не сбрасывается. function SetIntervalsCount(const Count : integer) : boolean; Устанавливает число интервалов. Возвращает False, если произошла ошибка. }

> property IntervalsCount : integer read fIntervalsCount;
>
> function SetIntervalsCount(const Count : integer) :
> boolean;

Этот момент непонятен. Вроде напрашивается

property IntervalCount: Integer read fIntervalsCount write SetIntervalsCount

Когда Setxxxx сделан функцией с результатом типа Boolean, это вызывает некоторую настороженность. Скорее, в случае неудачного выполнения стоит возбуждать исключение. Судя по тому, что функция Set работает исключительно с распределением памяти, EOutOfMemory будет возбуждено само.

Все методы работы с Count и массивом неких значений, которые ограничены этим Count стоит аккуратно переписать из класса TList - его неглупые люди писали, а не изобретать свои.

> [40] capkoh © (28.01.08 15:20)
> Я не стал делать из этого property, потому что непонятно,
> как вернуть ошибку.

Но так как [44] и [45], то можно использовать GetMem\FreeMem и забыть о контроле выделения памяти, что, в свою очередь, позволит реализовать свойство IntervalsCount, то есть [47]. Согласен с вами.

Думаю, пока это всё, что я хотел узнать.

Спасибо за помощь!

Оценка качества кода Найти похожие ветки