Качество кода

← →
Student © (2009-06-05 17:54) [0]

Здравствуйте. Я тока, тока учусь основам языка. Пишу "хорошую и очень полезную программу". Пытаюсь уделить особое внимание качеству кода. У меня возникло несколько организационных вопросов.
1) Программа написана в хорошем стиле ООП?
2) Правильно пытался использовать соглашения о наименовании?

На что следует обратить мне внимание, чтобы по максимуму повысить качество кода?

Вот ссылка на исходный проект программы: http://rghost.ru/279521
(архив chas.rar, 76Кб)
Спасибо

← →
Student © (2009-06-05 17:58) [1]

И, по возможности дополнительный вопрос. Все объекты в программы можно перемещать "правой мышью". Если перемещать мышку быстро(при нажатой правой кнопки), то объект "сбивается" и не перемещается до тех пор, пока не будет "фокуса" на объекте.
Если же вместо "правой мышки" переделать на "левую" - проблеммы как ни бывало. В чём может быть загвоздка?
Спасибо

← →
DVM © (2009-06-05 18:00) [2]

> Student ©

На первый беглый взгляд сносно, я думал хуже будет. Что вот сразу бросилось в глаза, так это:

lbl_XXXXX

Не по дельфийски это как то. Лучше без подчеркивания.
Не очень аккуратно отформатировано местами, пробелов не хватает вокруг :=, < , > И Т.Д.
WITH использовать бы не помешало.

← →
DVM © (2009-06-05 18:02) [3]

> Student ©

NEW()
Бла-бла-бла
DISPOSE()

лучше обернуть в Try...Finally тем более, что там у тебя между ними вероятны исключения.

← →
Игорь Шевченко © (2009-06-05 18:05) [4]

Я конечно извиняюсь, но программа написана безобразно.

> На что следует обратить мне внимание, чтобы по максимуму
> повысить качество кода?

на исходные тексты VCL, тексты примеров в Demos

← →
DVM © (2009-06-05 18:09) [5]

> Student ©

Вот еще:
http://www.delphikingdom.com/asp/viewitem.asp?catalogid=802

← →
Student © (2009-06-05 18:12) [6]

Пасибо огромное. :-) Буду исправляться :-)

← →
Rouse_ © (2009-06-05 18:16) [7]

На первый взгляд ужасно - про форматирование я просто опущу... но мемлики...

Конструкции следующаего плана выглядат достаточно забавно и оригинально:

New (PIni); PIni^ :=TIniFile.Create(ExtractFilePath(Paramstr(0))+"Chas.ini"); Dispose (PIni);

Ну и количесво конструкторов (много штук) явно превышает количество деструкторов (одна штука) найден над уникальнейшем по глубине смысла обработчике исключения:

finally StreamF.Free; end; except Exit; end;

Вывод - надо работать...

← →
DVM © (2009-06-05 18:23) [8]

> New (PIni);
> PIni^ :=TIniFile.Create(ExtractFilePath(Paramstr(0))+"Chas.
> ini");
> Dispose (PIni);
>

Действительно необычная запись.

← →
Student © (2009-06-05 18:26) [9]

Student © (05.06.09 17:58) [1]

> И, по возможности дополнительный вопрос. Все объекты в программы
> можно перемещать "правой мышью". Если перемещать мышку быстро(при
> нажатой правой кнопки), то объект "сбивается" и не перемещается
> до тех пор, пока не будет "фокуса" на объекте.
> Если же вместо "правой мышки" переделать на "левую" - проблемы
> как ни бывало. В чём может быть загвоздка?
> Спасибо

procedure TSSO.Что то MouseDown(Sender: TObject; Button: TMouseButton; Shift: TShiftState; X, Y: Integer); begin if Button <> mbLeft then Move:=false else begin Move:=true; X0:=x; Y0:=y; end; end;
Если так пишу - всё ок. Какие в этом случае будут рекомендации?
Спасибо

← →
Юрий Зотов © (2009-06-05 18:30) [10]

> Программа написана в хорошем стиле ООП

В плохом. Совершенно неверное создание динамических объектов, приводящее к утечкам памяти:
var PIni : ^TIniFile; ... // Выделили 4 байта: New (PIni); // Создали объект, но потеряли только что выделенные 4 байта: PIni^ :=TIniFile.Create(ExtractFilePath(Paramstr(0))+"Chas.ini"); ... // Если здесь возникнет исключение, Dispose не сработает // Освободили 4 байта из области памяти, занятой объектом, но при этом // потеряли остальную занятую им память размером TIniFile.InstanceSize-4: Dispose (PIni);

А надо так:
var Ini: TIniFile; ... Ini := TIniFile.Create(ExtractFilePath(Paramstr(0))+"Chas.ini"); try ... finally Ini.Free; end;
Есть и много других недостатков, но о них позже. Сначала разберитесь с этим. То, что Вы умеете работать с указателями - это просто отлично (и очень важно для будущего роста), мало кто из начинающих это умеет - но с объектами работают не так.

← →
Пит (2009-06-05 18:40) [11]

надо просто понимать, что TIniFile - как и любая переменная классового типа является уже указателем. Поэтому объявлять указатель PIni на класс TIniFile (который тоже является указателем) смысла нету.

Получается, объявления указателя на указатель. Прямая дорога запутаться нахрен.

← →
Student © (2009-06-05 18:45) [12]

Пасибо вам огромное за замечания. Буду исправляться. :-)

← →
Игорь Шевченко © (2009-06-05 18:54) [13]

Student © (05.06.09 18:45) [12]

Глупый вопрос можно ? А зачем наследоваться от картинок ?

TSSO = class (TImage)
TAddressIP = class(TImage)
TServ = class(TImage)
TAlarmClock = class(TImage)

← →
pasha_golub © (2009-06-05 19:00) [14]

Блин, люди оказывается даже посмотрели. Я думал вою про вирусы будет... :)

← →
DVM © (2009-06-05 19:05) [15]

> Игорь Шевченко © (05.06.09 18:54) [13]

> Глупый вопрос можно ? А зачем наследоваться от картинок
> ?

Я думаю, у него там че-то типа Visio. TServ - это изображение сервера, который можно таскать по форме.

← →
Student © (2009-06-05 19:06) [16]

> Игорь Шевченко © (05.06.09 18:54) [13]
>
> Student © (05.06.09 18:45) [12]
>
> Глупый вопрос можно ? А зачем наследоваться от картинок
> ?
>
> TSSO = class (TImage)
> TAddressIP = class(TImage)
> TServ = class(TImage)
> TAlarmClock = class(TImage)

Это нарушение LSP(Liskov Substition Principe). Я уже прочитал эту тему только что. Наследоваться нужно не для того чтобы повторно использовать код, а для того чтобы быть повторно используемым.
Пасибо за замечание вам.

← →
Юрий Зотов © (2009-06-05 19:13) [17]

> pasha_golub © (05.06.09 19:00) [14]

Там сырцы. Правда, еще ~ и dcu, но это уже по неопытности...
:o)

← →
Student © (2009-06-05 19:14) [18]

> DVM © (05.06.09 19:05) [15]
> Я думаю, у него там че-то типа Visio. TServ - это изображение
> сервера, который можно таскать по форме.

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

← →
Игорь Шевченко © (2009-06-05 19:26) [19]

Student © (05.06.09 19:14) [18]

> Нет. Это зделано для того, чтобы в будущем пользователи
> при начале использования программы, расставили красиво(поэстетичнее,
> так как надо) изображения

Я бы посоветовал фреймы сделать, которые можно "таскать" по главному окну, а уже во фреймах располагать нужный контент.

Они лучше Image, как минимум тем, что на них можно положить другие контролы.

← →
oldman © (2009-06-05 20:45) [20]

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

← →
Виктор85 (2009-06-06 17:27) [21]

> А надо так:
>
> var
> Ini: TIniFile;
> ...
> Ini := TIniFile.Create(ExtractFilePath(Paramstr(0))+"Chas.
> ini");
> try
> ...
> finally
> Ini.Free;
> end;

Я бы написал так)

var Ini: TIniFile; ... Ini := TIniFile.Create(ExtractFilePath(ParamStr(0)) + "Chas.ini"); try ... finally FreeAndNil(Ini); end;

← →
@!!ex_ (2009-06-06 17:41) [22]

> Я бы написал так)
>
> var
> Ini: TIniFile;
> ...
> Ini := TIniFile.Create(ExtractFilePath(ParamStr(0)) +
> "Chas.ini");
> try
> ...
> finally
> FreeAndNil(Ini);
> end;

У меня необычный вопрос... а нафиг?

← →
Виктор85 (2009-06-06 18:06) [23]

выработав привычку всегда использовать FreeAndNil, можно смело предполагать, что проверить - создан ли объект, достаточно выполнить проверку if Assigned()... я считаю хорошей привычкой после уничтожения объекта указатель устанавливать в nil, не оставляя там мусор

← →
Юрий Зотов © (2009-06-06 18:31) [24]

> Виктор85 (06.06.09 18:06) [23]

Причем особенно это важно для локальных переменных, каковой и должна быть переменная Ini.

Вот такой код встречать не приходилось?

i := 1;
if i = 1 then... // А вдруг не присвоилось?

← →
Виктор85 (2009-06-06 20:40) [25]

> Юрий Зотов © (06.06.09 18:31) [24]

Ну не до такой степени)

Примера дать не могу, выработал до автоматизма такое применение. Загляните в исходники Indy, там только так и освобождаются все объекты... в общем дело привычки, доказывать не буду)

← →
Юрий Зотов © (2009-06-06 20:59) [26]

> Виктор85 (06.06.09 20:40) [25]

Все правильно, это дело привычки. Но ведь привычки бывают и вредными?

FreeAndNil дает еще один вызов и еще одно присваиваивание - то есть, работает чуть медленнее. Конечно, это копейки, но на критических по скорости участках кода и копейки бывают важны. И если для глобальных переменных (или полей объектов) FreeAndNil действительно может оказаться полезным (а может и не оказаться), то для локальных становится просто бессмыссленным.

В общем, всяк овощ хорош в свое время. Но самая лучшая панацея - это аккуратный кодинг, тогда и FreeAndNil не понадобится. В данном же случае в коде автора вопроса (надеюсь, Вы его смотрели) СТОЛЬКО погрешностей и они НАСТОЛЬКО вопиющи, что говорить о замене Free на FreeAndNil пока что просто смешно.

← →
oldman © (2009-06-06 21:13) [27]

> Юрий Зотов © (06.06.09 18:31) [24]
> Вот такой код встречать не приходилось?
> i := 1;
> if i = 1 then... // А вдруг не присвоилось?

Мама родная!
Где ты это видел?

И почему перед присваиванием нет проверки на наличие объявления i?
И на наличие в природе 1?
И на совпадение типов?

← →
AndreyV © (2009-06-06 22:27) [28]

> [24] Юрий Зотов © (06.06.09 18:31)
> i := 1;
> if i = 1 then... // А вдруг не присвоилось?

i := 1; // Вдруг не проверилось?

Что с доступом к ДелфиМастер? Уже неделю только пару раз за день удаётся зайти, а на dmgate.k210.org вижу нормально.

> я считаю хорошей привычкой после уничтожения объекта указатель
> устанавливать в nil, не оставляя там мусор

Это плохая привычка. Забудь ее. При грамотном дизайне проверять объект на Assigned не приходится, лучше совершенствовать дизайн, чем совершенствовать привычку использовать костыли.

> AndreyV © (06.06.09 22:27) [28]
>
> > [24] Юрий Зотов © (06.06.09 18:31)
> > i := 1;
> > if i = 1 then... // А вдруг не присвоилось?
>
> i := 1; // Вдруг не проверилось?
>
> Что с доступом к ДелфиМастер? Уже неделю только пару раз
> за день удаётся зайти

Да всё в порядке с доступом. Ищи проблемы у себя.

> я считаю хорошей привычкой после уничтожения объекта указатель
> устанавливать в nil, не оставляя там мусор

Ничего полезного в этой привычке нет.

> [30] Германн © (07.06.09 01:05)
> Да всё в порядке с доступом. Ищи проблемы у себя.

Да я понимаю, что всё в порядке, о чём и написал о dmgate, но проблема скорее не у меня локально, а где-то дальше - потому и спросил - может известна.

← →
имя (2009-06-07 14:06) [33]

Удалено модератором
Примечание: Совсем совесть потерял?

← →
TIniFile (2009-06-07 14:23) [34]

> Юрий Зотов © (05.06.09 18:30) [10]
> var
> PIni : ^TIniFile;
> ...
> // Выделили 4 байта:
> New (PIni);
> // Создали объект, но потеряли только что выделенные 4 байта:
>
> PIni^ :=TIniFile.Create(ExtractFilePath(Paramstr(0))+"Chas.
> ini");
> ... // Если здесь возникнет исключение, Dispose не сработает
>
> // Освободили 4 байта из области памяти, занятой объектом,
> но при этом
> // потеряли остальную занятую им память размером TIniFile.
> InstanceSize-4:
> Dispose (PIni);

Фига-се новости...Освободили 4 байта из области памяти, занятой объектом, но при этом потеряли остальную занятую им память размером TIniFile.InstanceSize-4 - это из какого смысла?

> TIniFile (07.06.09 14:23) [34]

на самом деле потеряли больше чем TIniFile.InstanceSize-4, в нем еще и FileName хранится... ) так он и останется нефинализированный

← →
TIniFile (2009-06-07 14:33) [36]

> на самом деле потеряли больше чем

Это я понимать могу. Мне InstanceSize-4 чуть башню не снесло :) Затерли указатнль, потеряли весь объект без всяких там -4.

> TIniFile (07.06.09 14:33) [36]

Юра имел ввиду InstanceSize - SizeOf(Integer), тот самый Integer Dispose,которого , был сделан.

тьфу... ) не Integer, а ^IniFile :)

← →
TIniFile (2009-06-07 14:45) [39]

> Юра имел ввиду InstanceSize - SizeOf(Integer), тот самый
> Integer Dispose,которого , был сделан

Что он имел в виду я понял, только оно не соответствуе действительности.
Откуда взялся InstanceSize - SizeOf(Integer)?

New (PIni) - выделили SizeOf(Pointer) под указатель на объект.
PIni^ :=TIniFile.Create - Записали в эту выделенную память указатель на созданный объект. Сам объект со всем его InstanceSize лежит са-а-всем в другом месте.
Dispose (PIni) - освободили память, занятую указателем. Сам объект как был в другом участке памяти, так там и остался. Со всеми своими ссылками и со всем своим InstanceSize, целиком и без изъятия.

> TIniFile (07.06.09 14:33) [36]

New выделила 4 байта. Dispose их же и освободит, но только уже по другому адресу (т.к. значение указателя изменилось). Поэтому при Dispose теряем InstanceSize-4.

Общие же потери памяти составят InstanceSize (т.к. выделенные New 4 байта тоже были потеряны при создании объекта). А с учетом [35] - больше.

PS
Спокойнее, дружище, зачем так много эмоций?

Качество кода Найти похожие ветки