Как нельзя и как можно!
0. Нельзя черпать вдохновение на сайте
http://govnokod.ru/pascal, но можно и нужно задуматься о своем месте в мироздании. Возможно в качестве дворника, или кондуктора Вы принесете больше пользы человечеству.
1. Не следует приступать к работе в бессознательном состоянии когда ваш мозг не может совершать элементарную мыслительную деятельность, лучше лечь и поспать еще. Вот характерный пример:
1.
2.
3.
4.
5.
6.
begin
ds := tstringlist.Create;
ds.clear;
...
end;
Не нужно чистить только что созданный объект, а вот разрушать созданный локальный объект следует, при чем обязательно с использованием try finally. Помните сон разума рождает чудовищ.
2. Не забывайте, что парные процедуры BeginUpdate, EndUpdate, DisableControls, EnableControls и т. п. требуют try finally точно так же как и Create, Free
1.
2.
3.
4.
5.
6.
DataSet.DisableControls;
try
...
finally
DataSet.EnableControls;
end;
Если не использовать EnableControls, в блоке finally, то любое исключение приведет к потере работоспособности формы и вероятно потребует перезагрузить приложение. Для простого пользователя это бóльшая проблема чем утечка памяти из-за того, что некоторый объект не был бы разрушен.
3. Не следует использовать оператор GOTO, откройте для себя такое понятие как
структурное программирование . Требование может показаться очевидным, но тем не менее кто-то продолжает использовать этот оператор.
4.Не добавляйте и не оставляйте информацию, которая затрудняет анализ кода поиск нужной информации и массовое внесение изменений.
Не оставляйте закомментированные участки кода, лишние пустые строки, неиспользуемые переменные, самоочевидные комментарии и пр. Если в рамках текущей задачи нет времени на более тщательную проверку, то хотя бы добавляйте понятный комментарий в формате DoTo.
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
ClipBOard.AsText := ds.Text;
{ TODO : Здесь бы надо по-думать, но мне лень. Пусть это сделает кто-то другой вместо меня! }
// Len := Length(ws) shl 1 + 2;
// hData := GlobalAlloc(GMEM_MOVEABLE or GMEM_DDESHARE, Len);
// try
// pData := GlobalLock(hData);
// try
// Move(PChar(ws)^, pData^, Len);
// SetClipboardData(CF_UNICODEText, hData);
// finally
// GlobalUnlock(hData);
// end;
// except
// GlobalFree(hData);
// end;
Еще пример:
1.
2.
// Файл пустой, потому что нет ничего
FFiles.Delete(FileIndex);
Спасибо капитан Очевидность, мы знали, что если файл пустой, то в нем ни чего нет, хотелось бы знать какой файл, почему пустой, что в строке ниже удаляется...
5. Не смешивайте визуальное и не визуальное программирование. Например: если форма в файле dfm называется "Список клиентов", а где-то в коде оно меняется на "Список абонентов", то это несет дополнительные сложности, если нужно массово изменить все формы связанные с клиентами. Т.е. свойства которые необходимо изменять в коде должны меняться только в коде, а в dfm-файле должно быть установлено умолчательное значение. Тоже самое относится к тексту запросов и текстовым сообщениям.
6. Не используйте динамическое формирование запросов и макросы в запросах без крайней на то необходимости. Если запрос изменяется в коде, то невозможно его скопировать в сторонний редактор запросов и обратно. Требуется запускать приложение и мониторить все запросы. При этом нет гарантии, что вы пройдете по всем веткам программы и проверите все варианты запросов. Таким образом любое даже тривиальное изменение существенно усложняется.
7. Недопустимо чтобы после ваших изменений появлялись новые Hint`ы и Warning`и. Старые Hint`ы и Warning`и следует устранять, если они расположены около ваших изменений.
8. Нельзя бездумно обращаться к объектам внутри обработчика события. Что произойдет если это же событие будет присвоено другому объекту? Что будет если исходный объект кто-то скопипастит? Вот характерный пример:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
procedure TaReestr.gSearchResultSortMarkingChanged(Sender: TObject);
begin
if FColumnName <> '' then
begin
case gSearchResult.ColumnByFieldName(FColumnName).Title.SortMarker of
smDownEh:
tblreestr.SortOnFields(FColumnName,true,true);
smUpEh:
tblreestr.SortOnFields(FColumnName,true,false);
end;
end;
end;
Для грида gSearchResult был определен обработчик события по которому выполняется сортировка, в нем на прямую идет обращение к этому гриду и связанному с ним набору данных.
Во-первых в будущем набор данных может поменяться, а в событии будет по прежнему сортироваться старый набор данных и ни кто этого не заметит.
Во-вторых исходный грид вовсе не обязательно будет единственным. Он может быть скопирован вместе со всеми обработчиками событий (как и произошло в приведенном примере), но при клике по заголовку второго грида сортироваться будет набор данных из первого. Вариант исправления примерно такой:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
procedure TaReestr.gSearchResultSortMarkingChanged(Sender: TObject);
begin
if (FColumnName <> '') and (Sender is TBbsDBGridEh) and (TBbsDBGridEh(Sender).DataSource <> nil) and
(TBbsDBGridEh(Sender).DataSource.DataSet is TRxMemoryData) then
with TRxMemoryData(TBbsDBGridEh(Sender).DataSource.DataSet) do
begin
case TBbsDBGridEh(Sender).ColumnByFieldName(FColumnName).Title.SortMarker of
smDownEh:
SortOnFields(FColumnName, true, true);
smUpEh:
SortOnFields(FColumnName, true, false);
end;
end;
end;
Этот обработчик можно безопасно использовать в различных ситуациях. Можно было бы еще и переименовать его, чтобы было видно, что он не привязан к конкретному гриду.
Такой подход не может рассматриваться как общее требование, потому, что в некоторых случаях логика работы требует обращения к какому-то конкретному объекту, таким образом требуется обдумывать решение в каждом конкретном случае.
9. Нельзя обращаться к объектам, которые могут быть пустыми. Нельзя выполнять приведение типов, если нет уверенности в совместимости типов. Проверяйте значения на допустимость в обработчиках событий! Например:
1.
(sender as tbbsdbgrideh).datasource.dataset.first;
Очевидно, что DataSet может быть nil, DataSource может быть nil, Sender может быть не TBbsDBGridEh, т.е. в одной строке три потенциальных места для ошибки. Проверять можно как показано в предыдущем примере. В случае недопустимых, или пустых значений, в зависимости от логики работы можно либо ни чего не делать, либо поднимать исключение с человеческим текстом.
10. Не следует освобождать не инициализированные объекты.
1.
2.
3.
4.
5.
6.
7.
8.
9.
try
Clipboard := TCLipboard.Create;
ClipBoard.Open;
Clipboard.Clear;
ClipBOard.AsText := ds.Text;
finally
ClipBoard.Close;
ClipBoard.Free;
end;
Что будет, если ошибка возникнет в конструкторе? Clipboard будет содержать мусор, мы попытаемся обратится к этому объекту в разделе finaly, и получим другую ошибку, а информацию о первой ошибке потеряем. Что будет, если ошибка произойдет в ClipBoard.Close? Мы не уничтожим объект и получим утечку памяти. Вариант исправления:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
Clipboard := TCLipboard.Create;
try
ClipBoard.Open;
try
Clipboard.Clear;
ClipBOard.AsText := ds.Text;
finally
ClipBoard.Close;
end;
finally
ClipBoard.Free;
end;
Если в конструкторе произойдет ошибка, то в раздел finally мы не попадем. Общий шаблон использования объектов такой:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
Object1 := TObject1.Create;
try
Object2 := TObject2.Create;
try
...
finally
Object2.Free;
end;
finally
Object1.Free;
end;
или
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
Object1 := nil;
Object2 := nil;
try
Object1 := TObject1.Create;
Object2 := TObject2.Create;
...
finally
FreeAndNil(Object2);
FreeAndNil(Object1);
end;
11. Не следует вызывать один пользовательский обработчик события из другого события. Например:
1.
2.
3.
4.
5.
6.
procedure TaReestr.gSearchResultKeyDown(Sender: TObject; var Key: Word; Shift: TShiftState);
begin
inherited;
if (ssctrl in Shift) and ((Key = ord('C')) or (ord(Key) = ord('c')) or (Key = ord('С')) or (ord(Key) = ord('с'))) then
gSearchResultDblClick(Sender);
end;
Здесь по нажатию Ctrl+C происходит вызов обработчика двойного клика, который копирует данные в буфер обмена. Из названия gSearchResultDblClick ни как не следует, что обработчик этого события должен что-то скопировать в буфер обмена. Вполне возможно, в будущем к двойному клику будет привязана другая более стандартная функциональность, при этом и обработчик Ctrl+C перестанет работать. Лучше вынести код gSearchResultDblClick в отдельный метод и во всех обработчиках событий вызывать этот метод. А еще лучше создать действие (Action), и обрабатывать нажатия клавиш используя возможности Delphi, вместо того, чтобы изобретать свой собственный велосипед.
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
procedure TaReestr.CopyGridExecute(Sender: TObject);
var
I: Integer;
Control: TControl;
begin
inherited;
if ActiveControl is TBbsDBGridEh then
CopyBbsDBGridEh(TBbsDBGridEh(ActiveControl))
else if PageControl1.ActivePage <> nil then
for I := 0 to PageControl1.ActivePage.ControlCount - 1 do
begin
Control := PageControl1.ActivePage.Controls[I];
if Control.Visible and Control.Enabled and (Control is TBbsDBGridEh) then
begin
CopyBbsDBGridEh(TBbsDBGridEh(Control));
Break;
end;
end;
end;
12. Ни когда не освобождайте объект внутри метода этого объекта в Delphi это недопустимо. Также недопустимо уничтожение объекта внутри обработчика события, объекта, т.к. обработчики событий вызываются из методов объекта. Например нельзя уничтожать форму в обработчике события OnClose этой формы:
1.
2.
3.
4.
5.
procedure TaReestr.FormClose(Sender: TObject; var Action: TCloseAction);
begin
inherited;
Free;
end;
Здесь можно присвоить параметру Action значение caFree, но лучше всегда использовать стандартный шаблон работы с модальной формой Create-ShowModal-Free.
13. Не используйте вложенные with, не используйте большие блоки кода внутри with. Например:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
begin
with OMG do
begin
...
// несколько экранов кода
with WTF do
begin
...
// несколько экранов кода
...
end;
...
// несколько экранов кода
end;
end;
Надо сказать, что большие методы и блоки кода сами по себе затрудняют анализ кода и обычно являются дурным тоном а также признаком низкого профессионализма. Использование же with особенно сильно запутывает логику работы программы, при этом не позволяют использовать переход по ctrl+click и не позволяют видеть значения переменных в процессе отладки. Поэтому надо всегда избегать использования оператора with за исключением очень коротких и очевидных методов. Лучше вообще ни когда его не использовать.
14. Не используйте Free для глобальных переменных и полей
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
procedure TfrmShowPayDoc.aEditPayDocExecute(Sender: TObject);
begin
if frmEditPayDoc = nil then
begin
frmEditPayDoc := TfrmEditPayDoc.Create(Self);
try
frmEditPayDoc.gvPDocId:=...
...
finally
frmEditPayDoc.Free;
end;
end
else
try
frmEditPayDoc.gvPDocId:=...
...
finally
frmEditPayDoc.Free;
end;
end;
Здесь при первом вызове, объект frmEditPayDoc равен nil, затем он создается и разрушается. После Free переменная frmEditPayDoc содержит ссылку на разрушенный объект но не nil. Таким образом при повторном вызове будет обращение к разрушенному объекту и ошибка доступа к памяти. В исходном коде ошибка маскировалась большим количеством дублированного кода, который в примере не показан. Чтобы избежать таких ошибок используйте глобальный стандартный метод FreeAndNil, который разрушает объект и обнуляет значение переменной.
15. Избегайте дублирования кода, ибо "копипаст" — зло. См. пример из п. 14. Единственная разница в блоках if/else это создание объекта, если он был пустым. Таким образом очень легко переписать условие следующим образом:
1.
2.
3.
4.
5.
6.
7.
8.
if frmEditPayDoc = nil then
frmEditPayDoc := TfrmEditPayDoc.Create(Self);
try
frmEditPayDoc.gvPDocId := ...
...
finally
FreeAndNil(frmEditPayDoc);
end;
Но гораздо быстрее сразу писать без дублирования кода, чем потом пытаться найти отличия в двух ветках условия.
Вот еще характерный пример:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
if qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value <> NULL then
begin
if cbPaymDelay.Items.IndexOf(qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value) > 0 then
cbPaymDelay.ItemIndex := cbPaymDelay.Items.IndexOf(qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value)
else
begin
cbPaymDelay.ItemIndex := cbPaymDelay.Items.IndexOf(qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value);
if cbPaymDelay.ItemIndex = -1 then
begin
cbPaymDelay.Items.Add(qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value);
cbPaymDelay.ItemIndex := cbPaymDelay.Items.IndexOf(qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value);
end;
end;
end;
За длиннющим текстом "qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Value" теряется ясность понимания кода и что вообще предполагается сделать. Хотя если была цель сломать мозг своему коллеге, то стиль выбран правильно.
16. Не оставляйте Hint и Warning. Warning рекомендуется интерпретировать как ошибки компиляции.
1.
2.
3.
4.
5.
for i := 0 to qBalance.RecordCount - 1 do
begin
SumByBalances := SumByBalances + qBalance.FieldByName('ACCESS_$').AsFloat;
qBalance.Next;
end;
Здесь было предупреждение о том, что переменная SumByBalances может быть не инициализирована, но из-за того, что в проекте огромное количество предупреждений на это ни кто не обращал внимание.
1.
2.
3.
4.
5.
6.
SumByBalances := 0; // Обнуляем сумму!
for i := 0 to qBalance.RecordCount - 1 do
begin
SumByBalances := SumByBalances + qBalance.FieldByName('ACCESS_$').AsFloat;
qBalance.Next;
end;
Ошибка довольно серьезная, перед выполнением цикла значение суммы может содержать любое значение, но с большой вероятностью оно будет содержать 0, таким образом на этапе тестирования ошибка не будет замечена, однако при длительной работе там может содержаться любое значение и кто-то попадет на "лавэ" без каких-либо предупреждений (в 90-х за это отправляли раков кормить).
17. Не используйте стандартные свойства компонентов для хранения переменных. Весьма вероятно, что кто-нибудь их изменит, что сломает всю логику работы приложения.
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
21.
22.
23.
24.
25.
{ dfm }
object mPayments: TMenuItem
Caption = 'Платеж'
object N8: TMenuItem
Action = aGetPay
end
object N9: TMenuItem
Action = aAnnulPay
end
...
object aAnnulPay: TAction
Caption = 'Аннулировать'
...
OnUpdate = aAnnulPayUpdate
end
{ pas }
...
N9.Enabled := spCheckUser.ParamByName('O_RESULT').AsInteger = 1;
...
procedure TfrmMain.aAnnulPayUpdate(Sender: TObject);
begin
aAnnulPay.Enabled:= { разные другие условия } and (N9.Enabled);
end;
Здесь видно, что обработчик события aAnnulPayUpdate использует то значение N9.Enabled, которое сам же меняет. Это происходит не явно, и ошибку трудно найти. Можно просто добавить свойство с удобочитаемым названием и использовать его.
1.
2.
3.
4.
5.
6.
7.
8.
property AnnulmentEnabled: Boolean read FAnnulmentEnabled write FAnnulmentEnabled;
...
AnnulmentEnabled := spCheckUser.ParamByName('O_RESULT').AsInteger = 1;
...
procedure TfrmMain.aAnnulPayUpdate(Sender: TObject);
begin
aAnnulPay.Enabled:= { разные другие условия } and AnnulmentEnabled;
end;
Одна строчка написанного кода исключила бы ошибку и сделала бы код более ясным.
18. Не сравнивайте результат окна диалога с неумолчательным значением (обычно mrNo).
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
if MessageDlg('Вы хотите что-то сделать?', mtConfirmation, [mbYes, mbNo], 0) = mrNo then
{ ни чего не делаем }
else
{ что-то делаем }
...
if MessageDlg('Вы хотите что-то сделать?', mtConfirmation, [mbYes, mbNo], 0) <> mrNo then
{ что-то делаем }
...
if MessageDlg('Вы хотите что-то сделать?', mtConfirmation, [mbYes, mbNo], 0) = mrNo then
exit;
{ что-то делаем }
Что будет, если кто-то добавит третью кнопку mbCancel? Что будет если пользователь нажмет крестик в верхнем правом углу? Действие будет выполнено, что неожиданно и не правильно. Можно конечно сделать крестик недоступным и надеяться, что ни кто не добавить кнопку Cancel, но лучше переписать условия так:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
if MessageDlg('Вы хотите что-то сделать?', mtConfirmation, [mbYes, mbNo], 0) = mrYes then
{ что-то делаем }
else
{ ни чего не делаем }
...
if MessageDlg('Вы хотите что-то сделать?', mtConfirmation, [mbYes, mbNo], 0) = mrYes then
{ что-то делаем }
...
if MessageDlg('Вы хотите что-то сделать?', mtConfirmation, [mbYes, mbNo], 0) <> mrYes then
exit;
{ что-то делаем }
Общий шаблон для создания условий выглядит так:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
if { обычная ситуация } then
{ обычная работа }
else
{ необычная работа }
...
if not { обычная ситуация } then
exit;
{ обычная работа }
...
if { ситуация } <> { обычная ситуация } then
exit;
{ обычная работа }
19. Не используйте преобразования в строку и обратно без необходимости (в особенности это относится к датам). Во-первых это долгая операция, во-вторых увеличивает вероятность ошибок связанных с различиями в региональных настройках.
1.
2.
3.
4.
5.
6.
function TfrmGetPay.GetDateAndTimeBalA(vDate: TDateTime): TDateTime;
begin
if dtPayTime.Checked
then Result := StrToDateTime(DateToStr(vDate)+' '+TimeToStr(dtPayTime.Time))
else Result := StrToDateTime(DateToStr(vDate));
end;
Тип TDateTime это обычное число с плавающей точкой (Double), в целой части хранятся сутки (количество дней прошедших с 30.12.1899), в дробной части хранится время. Для получения целой и дробной части числа можно использовать например функции Trunc и Frac.
1.
2.
3.
4.
5.
6.
7.
function TfrmGetPay.GetDateAndTimeBalA(vDate: TDateTime): TDateTime;
begin
if dtPayTime.Checked then
Result := Trunc(vDate) + Frac(dtPayTime.Time)
else
Result := Trunc(vDate);
end;
Еще характерный пример.
1.
2.
3.
4.
var
vDate: TDateTime;
begin
vDate := StrToDateTime(FormatDateTime('dd.mm.yyyy 23:59:59', pWorkDate));
Здесь трудно с уверенностью сказать, какие мысли были в голове у автора, скорее всего см. п. 1, однако допустим, что автор хотел округлить дату и время до целых долей секунды. В реальности происходит преобразование даты и времени в строку в строго фиксированном формате, а обратно происходит преобразование из строки формат который указан в региональных настройках компьютера, что приводит к ошибке. Есть много вариантов округления, например такой:
1.
2.
3.
4.
5.
6.
var
vDate: TDateTime;
Y, M, D, H, N, S, Z: Word;
begin
DecodeDateTime(pWorkDate, Y, M, D, H, N, S, Z);
vDate := EncodeDateTime(Y, M, D, H, N, S, 0);
Полезными могут оказаться также функция Math.RoundTo и константы HoursPerDay, MinsPerDay, SecsPerDay, MSecsPerDay.
20. Ни когда не устанавливайте свойство формы Position в значение poDesktopCenter. Это означает, что форма будет расположена посередине рабочего стола, который представляет из себя прямоугольник охватывающий координаты всех имеющихся мониторов. При наличие двух мониторов форма будет расположена между ними, это неожиданно и не правильно, используйте значение Position = poScreenCenter.
21. Не следует забывать, что в выпадающем списке может быть выбрано недопустимое значение с ItemIndex = -1.
1.
if cbPaymDelay.Items.Strings[cbPaymDelay.ItemIndex] = qSomeFriendlyQueryName.FieldByName('Some_Friendly_Field_Name').Text then
Здесь в некоторых случаях будет ошибка. Которую трудно обнаружить на этапе тестирования.
22. Если изменяемое свойство некоторого объекта это другой объект, то нельзя просто ограничиться присваиванием ссылки.
1.
property Session: TSession read FSession write FSession;
Представим себе, что сначала присвоили некоторое значение, а потом удалили экземпляр объекта TSession. Получается, что поле FSession содержит не пустую ссылку на несуществующий объект. Обращение по такой ссылке приведет к ошибке при этом ни как не возможно проконтролировать допустимость обращения. Вот работающий шаблон:
1.
2.
3.
4.
5.
6.
7.
8.
9.
10.
11.
12.
13.
14.
15.
16.
17.
18.
19.
20.
21.
22.
23.
24.
25.
26.
protected
procedure Notification(AComponent: TComponent; Operation: TOperation); override;
published
property Session: TOraSession read FSession write SetSession;
...
procedure TMyClass.Notification(AComponent: TComponent; Operation: TOperation);
begin
inherited;
if (AComponent = FSession) and (Operation = opRemove) then
FSession := nil;
end;
procedure TMyClass.SetSession(const Value: TOraSession);
begin
if FSession <> Value then
try
if FSession <> nil then
RemoveFreeNotification(FSession);
FSession := Value;
if FSession <> nil then
FreeNotification(FSession);
except
FSession := nil;
raise;
end;
end;
Теперь если экземпляр объекта будет удален, то поле FSession будет содержать значение nil.
