Оцените код.

← →
@!!ex © (2007-11-22 17:13) [0]

Предложили попробоваться в весьма не кислую фирму.
Но для того, чтобы туда назначили собеседование, надо отослать им пример личного кода.
Я знаю, что у меня есть проблемы в плане качества кода.
Вот и хочеться услашыть мнение мастеров, чем плох этот код(кроме отсутствия комментов):
http://afera-net.narod.ru/AITypes.rar
Этот код реализует поведения всякой нечисти в мире.

← →
KSergey © (2007-11-22 17:17) [1]

Так вы в фирму-то и шлите.
Они и оценят. Что не понравится - если удасться - можно будет у них же и узнать.

← →
@!!ex © (2007-11-22 17:18) [2]

Так не интересно. :)
Предпочту узнать заранее, что в нем не так. ;)

← →
Celades © (2007-11-22 17:19) [3]

СРазу бросилось в глаза: обилие магических цифр.

← →
Судья (2007-11-22 17:19) [4]

> Предпочту узнать заранее, что в нем не так. ;)

Это нечестно перед работадателем.

← →
Правильный_Вася (2007-11-22 17:19) [5]

> код реализует поведения всякой нечисти в мире.

предлагаешь расплодить ее у нас на компах?

← →
@!!ex © (2007-11-22 17:20) [6]

> Это нечестно перед работадателем.

Ок. уговорили. :)

← →
vpbar © (2007-11-22 17:20) [7]

покритиковать. щааас. пока настроение есть.

← →
Черный Шаман (2007-11-22 17:21) [8]

> @!!ex © (22.11.07 17:13)
>
> Предложили попробоваться в весьма не кислую фирму.
> Но для того, чтобы туда назначили собеседование, надо отослать
> им пример личного кода.
> Я знаю, что у меня есть проблемы в плане качества кода.
> Вот и хочеться услашыть мнение мастеров, чем плох этот код(кроме
> отсутствия комментов):
> http://afera-net.narod.ru/AITypes.rar
> Этот код реализует поведения всякой нечисти в мире.

Паттерн Паблик-Морозов? Хотя для начинающего (1 год в серьезном программировании) нормально.

← →
@!!ex © (2007-11-22 17:23) [9]

> [8] Черный Шаман (22.11.07 17:21)

То что все в public"e лежит - это не ошибка.
Так нужно для дебага.

← →
Reindeer Moss Eater © (2007-11-22 17:29) [10]

ни одного гоуту, ни одного виз"а. некрасиво как-то.

← →
Kolan © (2007-11-22 17:29) [11]

1. Function  запомни все жирное с маленькой(кроме некоторых исключений типа Message в Exception)
2. Отвратный прием выравнимания по имени.
Constructor Create; Function GetAngle
3. LookAtTarget:boolean;
Boolean пишется с большой. И где пробел после «:»? Поля надо назаватьс буквы F.
Правильно так: FLookAtTarget: Boolean;
Кроме того что поля делают в public?

Далее у тебя поле названо глаголом Look а глаголы  это методы. Поля  это существительные. Оч. грубая ошибка.
Далее в коде их много&#133

4. AI_GoAway
Стиль с и с++ в Delphi недопустим  это я про подчеркивание.

5. Position,Target,MaxAngle, Angle
Между MaxAngle, Angle есть пробел а остальные кто будет ставить?

Правильно так: Position, Target, MaxAngle, Angle

6. Function GetAngle(const Position,Target,MaxAngle, Angle:TVector):TVector; override;
Заступ за черту 80  тоже минус, надо было перенести:
function GetAngle(const Position, Target, MaxAngle, Angle: TVector): TVector; override;
Опятьже после «:» надо ставить пробел всегла, а до него никогда. Ессно к оператору присвоения не относится.

7. effect_explode:TEmitter;
Вообще пипец. Почему стиль изменился? Почему он стал си образным.

8. single; integer;
Все это с большой. Все что не жирное с большой :)

9. TBuildingStatic
public
protected
Поменяны местами  плохо.

10. Реализацию не смотрел. Если исправишь выше описаное посмотрю.
LookAtTarget:=abs(AngleBetween)<pi/15; Result.X:=0;
Опятьже пробелов не хватает. Abs, Pi c маленькой
Надо:
LookAtTarget := Abs(AngleBetween) < Pi/15; Result.X := 0;

Общее впечатление  плохое в основном из-за толпы не контролируемых public и published полей.
И непонятных названий, особенно глаголы в полях&#133

← →
@!!ex © (2007-11-22 17:32) [12]

> Правильно так: FLookAtTarget: Boolean;
> Кроме того что поля делают в public?

Писать отдельную обертку типа:
property IsOnTarget:Boolean read FLookAtTarget write FLookAtTarget;
А смысл?

← →
Kolan © (2007-11-22 17:33) [13]

Увидел еще:
procedure TDestroyer.Update(dt: integer);

Это вообще из вредных советов, грубая ошибка, очень. Что за dt? Почему не е001 или hte?

← →
@!!ex © (2007-11-22 17:34) [14]

> [13] Kolan © (22.11.07 17:33)

DelayTime.. это даже не моя идея.
Сколько видел проектов больших игр, демок, везде dt....

← →
Kolan © (2007-11-22 17:35) [15]

> А смысл?

Это много говорит о тебе. Смысл в инкапсуляции и тем что ты ей не пользуешься. Уверен что если посидеть, то можно найти толпу мест где поля должны быть доступны только для чтения, но ты это контролируешь у себя в голове а не в коде&#133 А голова у тебя такая одна и если я сяду за твой ког я могу все испротить&#133

LookAtTarget

Объясни что это значит по русски, я предложу имя для переменной.

← →
@!!ex © (2007-11-22 17:37) [16]

Типа так:
TAIIntercept = class(TAI)
protected
FIsGoAway:Boolean;
FIsLookAtTarget:Boolean;
FDistance:Single;
public
Constructor Create;
Function GetAngle(const Position,Target,MaxAngle, Angle:TVector):TVector; override;
property IsGoAway:Boolean read FIsGoAway;
property IsLookAtTarget:Boolean read FIsLookAtTarget;
property DistanceToTarget:Single read FDistance;
end;

← →
Kolan © (2007-11-22 17:37) [17]

> DelayTime&#133 это даже не моя идея.

А вот я не видел игр, но знаю что dt  это дэтэ и не чего более. А DelayTime  время задержки.

Чтобы писать нормальный код надо использовать только 1(одни) прием. &#133 Надо писать код не для себя, а для другого программиста, который его будет читать не зависимо от того, случится это или нет&#133

← →
@!!ex © (2007-11-22 17:37) [18]

> Это много говорит о тебе. Смысл в инкапсуляции и тем что
> ты ей не пользуешься. Уверен что если посидеть, то можно
> найти толпу мест где поля должны быть доступны только для
> чтения, но ты это контролируешь у себя в голове а не в коде…
> А голова у тебя такая одна и если я сяду за твой ког я
> могу все испротить…

Я уже понял. Согласен на 100%.

← →
Riply © (2007-11-22 17:38) [19]

> [11] Kolan © (22.11.07 17:29)
> 4. AI_GoAway
> Стиль с и с++ в Delphi недопустим — это я про подчеркивание.

"А мужики то и не знают" и пишут: ListView_SetItemCount :)

← →
@!!ex © (2007-11-22 17:39) [20]

Кстати. еще вопрос, наличие Destroy обяхательно?
Где-то недалеко отсюда слышал, что Destroy должен быть всегда, даже если он пуст

← →
Kolan © (2007-11-22 17:39) [21]

> property IsGoAway:Boolean read FIsGoAway;
> property IsLookAtTarget:Boolean read FIsLookAtTarget;
>
> property DistanceToTarget:Single read FDistance;

Я незнаю смысл может эти поля и должны быть доступны для чтения/записи, может нужны сеттеры геттеры, это по итуации, но одно из простых rule of thumb  все поля приватные! Еснно можно отступать от этого, но ввиде исключения, а у тебя наоборот&#133

← →
@!!ex © (2007-11-22 17:40) [22]

> [19] Riply © (22.11.07 17:38)

Кстати, причина того, что я там поставил подчервикание - чтобы отделить AI от GoAway.
Но сама приставка AI тут явно лишняя, поэтому и уточнять не стал, исправил просто.

← →
@!!ex © (2007-11-22 17:40) [23]

> [21] Kolan © (22.11.07 17:39)

Так эта.. я не понял, правиьно или нет?

← →
Kolan © (2007-11-22 17:41) [24]

> «А мужики то и не знают» и пишут: ListView_SetItemCount
> :)

Вот такие дела&#133 подчеркивание  это стиль си, который предложили, &#133 как там их Керниган и Ричи(кажется).
А на Delphi пишут используя CamelCase. Кроме того CamelCase стандарт UML. И в Jave принят тот же стиль(кроме геттеров и может еще чего я не спец), и в C#.

← →
@!!ex © (2007-11-22 17:42) [25]

Почему выравнивание по имени - это плохо?
Вроде так проще читаеться..

← →
Kolan © (2007-11-22 17:43) [26]

> Так эта&#133 я не понял, правиьно или нет?

Если тебе не надо давать коиенту писать в поле, то не давай тут нет правильно/неправильно, тут еть как надо, а для ответа на этот вопрос мне надо разобраться в коде, понять что кто делает&#133

А вот названия могу предложить, если опишите что это значит на русском.

← →
Riply © (2007-11-22 17:44) [27]

> [24] Kolan © (22.11.07 17:41)

Ничего не могу возразить, кроме того, что привыкла ориентироваться на "генофонд".
Раз там используют - значит можно :)

← →
Kolan © (2007-11-22 17:44) [28]

> Почему выравнивание по имени  это плохо?
> Вроде так проще читаеться&#133

Это не проше, это не правиьно, это не соотв обепринятому стилю VCL http://www.citforum.ru/programming/delphi/style_delphi/.

Раз там используют Очень редко посмотрите. Подчеркивание использется для служебных целей:
_AddRef напимер специально с подчеркивания, чтобы было ясно что его трогать не надо(compiler automatically generates a call to _AddRef).

TAIIntercept = class(TAI) protected FIsGoAway:Boolean; FIsLookAtTarget:Boolean; FDistance:Single; public Constructor Create; Function GetAngle(const Position,Target,MaxAngle, Angle:TVector):TVector; override; property IsGoAway:Boolean read FIsGoAway; property IsLookAtTarget:Boolean read FIsLookAtTarget; property DistanceToTarget:Single read FDistance; end;
Классик с помощью метода GetAngle расчитывает направление движения объекта. Например, поворачивает истребитель на цель.
IsGoAway - отвечает на вопрос, заходит истребитель на цель, или наоборот летит от нее, чтобы отлетев на достаточное расстояние - развернуться на следующий заход.
IsLookAtTarget - указывает, нацелен ли сейчас объект на цель или же нет. Нацелен - это значит что вектор по направленю к цели, и вектор движения объекта меньше заданного.
DistanceToTarget - расстояние до цели.

IsAimedOnTarget

> 7. effect_explode:TEmitter;
> Вообще пипец. Почему стиль изменился? Почему он стал си
> образным.

Это не си образный стиль.
Это отделение префикса.
поскольку есть префиксы, например:
effect
texture
model
то было решено, чтобы проще оринетироваться - писать их с маленькой буквы и отделять подчеркиванием.

P.S>
Совместимость с VCL значения не имеет, он здесь не используется.

> Типа так:

Нет типа так:

TAIIntercept = class(TAI) private FIsGoAway: Boolean; FIsLookAtTarget: Boolean; FDistance: Single; public constructor Create; //большая буква function GetAngle(const Position, Target, MaxAngle, Angle: TVector): TVector; override; // Перенос, пробелы, большая буква property IsGoAway: Boolean read FIsGoAway; // Пробелы property IsLookAtTarget: Boolean read FIsLookAtTarget; // Пробелы property DistanceToTarget: Single read FDistance; // Пробелы end;

И названия исправить.

Почему private?

> Совместимость с VCL значения не имеет, он здесь не используется.

Тогда вам на сайт того, на чем вы пишите.

Я могу прокоментить только то, как принято писать на Delphi я это стиль VCL.

Сравните
[31] IsAimedOnTarget

И ваше

LookAtTarget

Я понял первое как «Нацелена на цель» Да/Нет, второе «посмотреть/посмотри на цель»

> то было решено, чтобы проще оринетироваться  писать их
> с маленькой буквы и отделять подчеркиванием.

Это исеть чистый си стиль. все с маленькой а слова отделяются «_»

> Сравните
> [31] IsAimedOnTarget
>
> И ваше
>
> LookAtTarget

Ок.

> Почему private?

Пашта они скрыты должны быть, пашта инкапсуляция&#133

> [37] Kolan © (22.11.07 17:55)

Как то не очень скрыто получилось:
if AI.FIsAimedOnTarget and (LastShoot<=0) and (AI.FDistance<400) and (Action=GOA_ATTACK) then begin
Вполне успешно компилится, хотя вызывается из куска кода, который никакого отношения к TAIIntercept не имеет...

по поводу форматирования. - есть непривычности. Но DelForEXP все исправляет. Так что это мелочи, имхо.
Хотя if Dist<MaxDistance then begin сам так делаю. Но некоторым не нравится. :)
...
if not Dead then - лучше не инвертировать. Т.е. сначала ветка true, потом false. В данном случае может лучше не Dead а Аlive?
if (Sleep=0) and (not Dead) then begin if not Dead and WorldMap.Objects.ObjectCorrect(Target) and (Target.Life>0) and (target.Model<>nil) then begin - перестраховываешься? :) И уж очень много условий. Тогда форматровать красивее надо.

> procedure TBomb.Draw;
> procedure TBomb.Update(dt: integer);
Эти и еще некоторые методы не выполнятся если Dead=true. Так может ставить проверку в начале if dead then exit; ?
Некоторые Draw настолько похожи. Почему не запихнуть общее в функцию. Или Draw сделать у базового класса TGameObject и этим кодом?

> перестраховываешься? :) И уж очень много условий. Тогда
> форматровать красивее надо.

Не перестраховка, скорее всего менял код и не углядел, что проверка уже есть...
Смысл от перестраховки такой? :)

> Хотя if Dist<MaxDistance then begin сам так делаю. Но
> некоторым не нравится. :)

Что плохо?

> Эти и еще некоторые методы не выполнятся если Dead=true.
> Так может ставить проверку в начале if dead then exit; ?

Dead - это вообще команда контейнеру, что объект можно удалять.
Поэтому после того как Dead - true объект живет максимум еще один тик.

> Некоторые Draw настолько похожи. Почему не запихнуть общее
> в функцию. Или Draw сделать у базового класса TGameObject
> и этим кодом?

думал над этим, но придумать не мог. Спасибо этой ветке и комментаторам(и тебе лично, твой пост и помог) идея появилась.
Как раньше до нее не додумался - х.з.
Наверно это потому, что в ВУЗе учили не использовать exit...

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