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

← →
capkoh (2008-01-18 13:04) [0]

Довольно давно интересует вопрос: как же можно оценить качество кода? Исходного, конечно. Наверняка этот вопрос уже здесь задавался не раз, но видимо, ушёл в архив.

По пунктам:
1. Существуют ли какие-либо методики оценки качества кода? Например, тестовые задания или что-то в этом роде?
2. Могу ли я, не имея каких-либо навороченных специальных для этого систем, сделать достаточно достоверную оценку?

Я хотел бы повысить качество своего кода, но я не знаю ЧТО мне нужно для этого в нём менять...
Подскажите, на что ориентироваться, что почитать или посмотреть.

← →
gpsNeeder (2008-01-18 13:05) [1]

почитай из DelphiWorld

%DelphiWorldFolder%\base\code_control.html

← →
Ins © (2008-01-18 13:09) [2]

> Я хотел бы повысить качество своего кода, но я не знаю ЧТО
> мне нужно для этого в нём менять...

У Макконелла есть книга "Совершенный код"

← →
ZoldBerger (2008-01-18 13:12) [3]

С. Макконнелл

Совершенный код
_______________
Практическое руководство по разработке программного обеспечения
Глава 32 Самодокументирующийся код

← →
Игорь Шевченко © (2008-01-18 13:12) [4]

> Существуют ли какие-либо методики оценки качества кода?

Да, в BDS2006 есть Code Quality Expert (на закладке Model View проекта)
Соответственно, гугль по слову Code Quality Metrics выдаст кучу методик оценки качества кода

← →
Юрий Зотов © (2008-01-18 13:32) [5]

> как же можно оценить качество кода

Так же, как оценивается выступление фигуристов, гимнастов и т.п.

Есть некие критерии, по ним и оценивается. Но эти критерии часто бывают видны только опытному глазу (как рядовой зритель не видит мелких помарок в выступлении фигуриста, а судья видит; как рядовой зритель не может оценить сложность исполненных элементов, а судья может).

Часть этих критериев поддаются формализации, поэтому их можно заложить в программу оценки качества кода.

← →
ketmar © (2008-01-18 16:34) [6]

нечего тут изобретать. достаточно дать почитать код постореннему программисту. если плюётся и кричит «что за лапша! ни@%@$^ не понятно!» — то код гуано.

← →
isasa © (2008-01-18 16:37) [7]

ketmar © (18.01.08 16:34) [6]

а если посторонний программист, как программист - гуано ?
Шо будем делать?

← →
Kolan © (2008-01-18 16:38) [8]

> Да, в BDS2006 есть Code Quality Expert (на закладке Model
> View проекта)

Открыл Model View, куда смотреть  невижу&#133

← →
Павел Калугин © (2008-01-18 16:45) [9]

есть такой предмет "метрология"
и есть в нем 2 подхода
1. отечественный - група бородатых дядек (экспертов) смотрят и оценивают
2. западный - плучаем цифирки по разным характеристикам и сравниваем. Навскидку есть метрики интеллектуальности кода, метрики сложности, и так далее (например читать тут http://met-rix.narod.ru/page1.htm)

← →
Джо © (2008-01-18 16:55) [10]

> [7] isasa © (18.01.08 16:37)
> ketmar © (18.01.08 16:34) [6]
>
>
> а если посторонний программист, как программист - гуано
> ?
> Шо будем делать?

Увольнять без выходного пособия :)

← →
Kerk © (2008-01-18 16:58) [11]

СPoint_05_07_2007 constant date := To_Date("20.11.2007","DD.MM.YYYY");

Пример хорошего самодокументирующегося кода :)

← →
ketmar © (2008-01-18 17:59) [12]

>[7] isasa ©(18.01.08 16:37)
убивать.

← →
Kolan © (2008-01-18 18:04) [13]

Раскажите лучьше про Code Quality Expert &#133

← →
Павел Калугин © (2008-01-18 18:13) [14]

> [13] Kolan © (18.01.08 18:04)

Почитай сначала про метрики чтобы понять что он меряет

← →
Kolan © (2008-01-18 18:15) [15]

> Почитай сначала про метрики чтобы понять что он меряет

Я хочу инструмент увидеть, который невидел, а про метрики &#133 :)

← →
Kostafey © (2008-01-18 23:02) [16]

> [2] Ins © (18.01.08 13:09)

Пасибо, книжка будет полезной.

← →
Павел Калугин © (2008-01-19 01:26) [17]

И смысл смотреть на таблице метрик не понимая что эти цифирки значат? там не всегда большое число хорошо а маленькое плохо.

← →
Kolan © (2008-01-19 11:51) [18]

> И смысл смотреть на таблице метрик не понимая что эти цифирки
> значат? там не всегда большое число хорошо а маленькое плохо.

да где смотреть то&#133

← →
capkoh © (2008-01-19 16:32) [19]

Да, спасибо, книжку посмотрю.

Метрики и спецсредства оно, конечно, хорошо, но есть ли практические рекомендации для того, чтобы посмотреть на код и сказать, что это плохой код? Или хороший. Дать другому программисту можно на прочтение, но где гарантии, что этот программист сможет адекватно оценить чужой код? Т.е. в большей степени меня интересует даже не качество кода с точки зрения метрик, а читабельность и понятность кода. Я думаю, что они довольно тесно связаны с качеством кода, но, безусловно, более субъективны.

← →
Mike Goblin (2008-01-19 17:43) [20]

Добрый день

> меня интересует даже не качество кода с точки зрения метрик,
> а читабельность и понятность кода

По поводу читабельности кода существует описание стиля кодирования для Object Pascal - http://dn.codegear.com/article/10280
Начните с него.

PS Немного стариковского ворчания (все нижеследующее есть только мое личное мнение, не претендующее на истину в последней инстанции)

> есть ли практические рекомендации для того, чтобы посмотреть
> на код и сказать, что это плохой код? Или хороший.

Таких рекомендаций нет, так как понятие "плохой код" неоднозначно и может разными людьми трактоваться очень по-разному. Например, может иметься ввиду оформление кода, количество ошибок, повторная используемость и простота сопровождения другими программистами итд.
Первое слово "плохой" - человек с техническим складом ума обычно преобразует в "удовлетворяет требованиям" и начинает искать способы формализации требований на основе измеряемых показателей.
Второе слово "код", имеет несколько контекстов. Первый из них непосредственно текст программы (исходный код) и его оформление, второй контекст - это структура модулей, процедур, классов т.е аспекты, относящиеся больше к проектированию, нежели к кодированию.

← →
capkoh © (2008-01-19 18:27) [21]

> Первое слово "плохой" - человек с техническим складом ума
> обычно преобразует в "удовлетворяет требованиям" и начинает
> искать способы формализации требований на основе измеряемых
> показателей.

Именно поэтому я и хотел увидеть некоторые эмпирические правила от профессионалов, потому что, даже если код удовлетворяет этим измеряемым показателям, это вовсе не значит, что он читабелен и понятен субъективно. Впрочем, ссылка – именно то, что я хотел увидеть. И, да, на данный момент меня интересует сам исходный код.

Следующий вопрос не менее интересен. По крайней мере, для меня. Где граница между процедурным и объектно-ориентированным программированием? Как выявить связь между «вторым контекстом» слова «код» и решением этого вопроса? И как принимать решения на основе этих связей?

← →
Mike Goblin 2 (2008-01-19 21:53) [22]

Интернет что ли глючит :(((
Вдогонку к предыдущему
http://www.citforum.ru/programming/delphi/style_delphi/
То же, но на русском

← →
Mike Goblin (2008-01-19 22:00) [23]

> я и хотел увидеть некоторые эмпирические правила от профессионалов

Лично у меня сложился следующий набор эмпирических правил оценки оформления кода:
1. Следование стилю кодирования, близкому к рекомендованному/стандартному для средства разработки/языка.
1.1 Названия переменных и методов на английском языке (а не по-русски английскими буквами).

2. Наличие комментариев ( сие изложено часто в п.1, но ...)
2.1 В заголовке модуля. Как минимум - Назначение модуля.
2.2 Для классов и методов экспортируемых модулем (доступных снаружи), лучше для всех процедур/методов/классов модуля. Описание параметров, возвращаемого значения, исключительных ситуаций, значимых особенностей алгоритма.
2.3 Наличие комментариев в теле методов класса/процедурах.
2.4 Отсутствие комментариев в тривиальных местах (например, комментариев по поводу переменной-счетчика цикла for).

3. Беглая не автоматизированная (на глаз) оценка по некоторым метрикам кода (это частично не оформление, а уже кодирование и даже чуть-чуть проектирование) + любимые грабли
3.1 Количество строк модуля - от 3-х экранов до 10 экранов текста. Меньше - плохо т.к ведет к излишнему количеству модулей. Больше тоже плохо - один "жирный" модуль трудно сопровождать/модифицировать.
3.2 Кол-во строк в методах. Один-два экрана. Причины - см 3.1
3.3 Высокая цикломатика (большое количество ветвлений ) в теле метода. Трудно тестировать все ветки. Трудно понимать алгоритм.
3.5 if else для возврата буленовского значения метода
3.6 short curcuit conditions и методы с побочными эффектами
3.7 Пустые обработчики исключительных ситуаций и finally блоки

4. Тестовый прогон компилятором
4.1 Отсутствие синтаксических ошибок
4.2 Отсутствие неиспользуемых, но импортируемых внешних модулей (компилятор как правило выдает предупреждение об этом).
4.3 Желательно отсутствие других предупреждений компилятора.

← →
capkoh © (2008-01-20 22:02) [24]

Спасибо!

← →
Mike_Goblin (2008-01-21 10:48) [25]

В контексте нашего разговора почитайте про рефакторинг. Есть хорошая книга Фаулер, Бек Рефакторинг: улучшение существующего кода

← →
Игорь Шевченко © (2008-01-21 10:56) [26]

> но есть ли практические рекомендации для того, чтобы посмотреть
> на код и сказать, что это плохой код?

Выкладывай

← →
capkoh © (2008-01-21 15:12) [27]

Вот такой небольшой юнит. Написан задолго до начала этой ветки, поэтому недочёты, конечно, есть. Я долго думал, нужно писать или нет, зачем нужен TListTimer, но решил не писать. Это нарушит чистоту эксперимента. Так как если оценивать с точки зрения читаемости, то нужно оценить и понятность. Код мой, если что.

Просьба замечания обосновывать, если это несложно...

unit ListTimer; interface uses Windows; type PIntervalsArray = ^TIntervalsArray; TIntervalsArray = array [0..0] of cardinal; type TListTimerProc = procedure (const ElapsedIntervalIndex : WORD); type TCallbackTimerProc = procedure (const hwnd : HWND; const uMsg : UINT; const idEvent : UINT; const dwTime : DWORD); stdcall; type PListTimer = ^TListTimer; TListTimer = class private fTimerWnd : hWnd; fQuartzID : cardinal; fElapsed : cardinal; pIntervals : PIntervalsArray; public LoopTimer : boolean; TimerProc : TListTimerProc; CurrentInterval : WORD; IntervalsCount : WORD; constructor Create(const IntCount : WORD; const hWnd : HWND); destructor Destroy(); override; procedure SetInterval(const Index : WORD; const Duration : cardinal); procedure Start(const FromCurrentPos : boolean); procedure Stop(); end; implementation procedure CallbackTimerProc(const hwnd : HWND; const uMsg : UINT; const idEvent : UINT; const dwTime : DWORD); stdcall; var TargetTimer : TListTimer; CurrentDuration : cardinal; begin // Amazing... TargetTimer := TListTimer(idEvent); inc(TargetTimer.fElapsed); // Duration of currently running interval CurrentDuration := TargetTimer.pIntervals[TargetTimer.CurrentInterval]; if TargetTimer.fElapsed > CurrentDuration then TargetTimer.fElapsed := CurrentDuration; if TargetTimer.fElapsed = CurrentDuration then begin inc(TargetTimer.CurrentInterval); if (TargetTimer.CurrentInterval = TargetTimer.IntervalsCount) then if TargetTimer.LoopTimer then TargetTimer.CurrentInterval := 0 else KillTimer(TargetTimer.fTimerWnd, TargetTimer.fQuartzID); if Assigned(TargetTimer.TimerProc) then TargetTimer.TimerProc(TargetTimer.CurrentInterval); TargetTimer.fElapsed := 0; end; end; constructor TListTimer.Create(const IntCount : WORD; const hWnd : HWND); begin fTimerWnd := hWnd; fQuartzID := cardinal(Self); IntervalsCount := IntCount; GetMem(pIntervals, IntervalsCount * sizeof(cardinal)); end; destructor TListTimer.Destroy(); begin KillTimer(fTimerWnd, fQuartzID); FreeMem(pIntervals, IntervalsCount * sizeof(cardinal)); inherited; end; procedure TListTimer.SetInterval(const Index : WORD; const Duration : cardinal); begin if Index < IntervalsCount then pIntervals^[Index] := Duration; end; procedure TListTimer.Start(const FromCurrentPos : boolean); begin fElapsed := 0; if not FromCurrentPos then CurrentInterval := 0; if IntervalsCount > 0 then SetTimer(fTimerWnd, fQuartzID, 40, @CallbackTimerProc); end; procedure TListTimer.Stop(); begin KillTimer(fTimerWnd, fQuartzID); end; end.

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

1. Не нравится.
2. Непонятно, зачем нужен код
3. Непонятно, зачем нужно объявление PListTimer = ^TListTimer;

судя по всему, нужен он для того чтобы юзать один таймер для нескольких событий, отличающихся интервалом.
Хотя, я бы назвал TMultipleTimer или TMultiTimer
потому что List предполагает именно список неких объектов, а не просто массив целых чисел.
Зачем объявлен указатель на класс, и правда непонятно

← →
ZoldBerger (2008-01-21 16:26) [30]

Удалено модератором

Скоро будет «работа над ошибками».

Правильно ли я составил план улучшения?

Его состав:
1. Добавить в начало модуля описание класса и его методов.
2. Сделать проверку на успешность выполнения SetTimer().
3. Учесть был ли установлен таймер при вызовах KillTimer().
4. Перенести параметр LoopTimer в функцию Start().
5. Сделать свойство IntervalsCount для динамического изменения длины списка интервалов (Get\SetIntervalsCount). Запретить изменение этого свойства во время работы таймера. Контролировать указанное значение IntervalsCount на вхождение в допустимый диапазон.
7. Контролировать выделение и удаление памяти под список (отказ от Get\FreeMem).
8. Добавить чуть-чуть комментариев в коде.
9. Убрать всё барахло.

Я всё же хочу довести до ума хотя бы один юнит.
Прошу вас комментить только по делу.

> Его состав:
> 1. Добавить в начало модуля описание класса и его методов.
>

Правильно

2-7 не знаю

8. Правильно
9. Вдвойне правильно

Скажу как оценивается качество кода (C#, VB.NET) у нас:
1. Code Review (делается тим лидом)
2. FxCop + FxCop Guidance
3. Внутренняя утилита, которая прямо в редакторе VS подчеркивает и подсказаывает, что где и как накосячил
4. У некоторых есть лицензии на ReSharper, он тоже здорово помогает в соблюдении стандартов.

> [27] capkoh © (21.01.08 15:12)

Замечаний много:
- зачем нужны GetMem, FreeMem, PIntervalsArray, если все элементарно делается с помощью динамических массивов?
- где здесь используется тип TCallbackTimerProc?
- зачем нужно PListTimer = ^TListTimer?
- что делают поля в секции public? В образцово-показательном коде это недопустимо.
- вместо TimerProc : TListTimerProc следует сделать property OnTimerProc: TListTimerProc;
- вместо procedure SetInterval в public удобнее (но далеко не обязательно) использовать property Interval, а SetInterval сделать его сеттером.
- зачем в FreeMem указывать размер памяти? Недоверяешь менеджеру памяти?
- в разных местах используется разный стиль работы с pIntervals
- неудачное название класса. Первое, что приходит в голову - объект работает со списком объектов TTimer.
- зачем в CallbackTimerProc() везде const стоит?
- очень мало комментариев.

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

Loginov Dmitry © (26.01.08 00:58) [35]

> - зачем нужны GetMem, FreeMem, PIntervalsArray, если все
> элементарно делается с помощью динамических массивов?

А зачем здесь динамические массивы ?
В чем выигрыш ?

> А зачем здесь динамические массивы ?
> В чем выигрыш ?

Выигрыш - в сокращении кода на 3 строчки. Больше - никакого.

Теперь более серьезное замечание:
почему после GetMem() нет FillChar()?
Отсутствие обнуления приведет к ложным срабатываниям таймера.

Спасибо за отзывы! Завтра выложу исправленную версию.

{*******************************************************} { } { 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, потому что непонятно, как вернуть ошибку.

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