07-18-2023, 05:54 AM
(This post was last modified: 07-18-2023, 07:48 PM by Alexandre Machado.)
An IntraWeb user asked me some help to refactor an old IW application that he is maintaining, written by someone else. I asked him permission to publish parts of the code because I find it valuable to learn a thing or two.
This is the original code (with some variable name changes so it can't be back traced):
He asked me what I would consider to change in code like this.
Here's my code review rant list
1- There is no reason to use WideStrings. WideString should be used in COM and when calling Windows API directly. Other than that, no reason.
2- Setting the SQL: Each time qryList.SQL.Add() is called, a whole bunch of methods and events are triggered. Some events may even trigger Database requests (we never know). Don't do this. Set the SQL at once using a single Add() or, what I do, qryList.SQL.Text := 'bla bla bla'
3- String concatenation in a loop: Always a bad idea. You can get better performance using something like our TStrBuilder (declared in unit IW.Common.StrBuilder)
4- Using FieldByName() inside a tight loop. FieldByName performance has improved but it won't ever be as efficient as having the field reference to use directly. Use FieldByName() 1 time to get the field reference and then use it inside the loop.
5 - DataSet loop: As a rule of thumb, use DataSet.DisableControls..DataSet.EnableControls when looping a DataSet. You possibly don't know the implementation details of the DataSet. In this case you don't know if DisableControls will improve the performance or not. It does make a big difference in ADO DataSets, for instance.
6- One query field is not used at all
7 - Several statements have no reason to exist. Query.Close before destroying it, SQL.Clear just after creating the query, and various others.
Tomorrow I'll publish the code I wrote to replace this, keeping the same functionality and method signature.
This is the original code (with some variable name changes so it can't be back traced):
Code:
function CreateSelectHtml(var idComboBox, key, year, enabled, action: String):String;
var
html: WideString;
qryList: TFDQuery;
begin
Result := '';
qryList := TFDQuery.Create(Nil);
qryList.Connection := Controller.ConexaoEscola;
try
qryList.close;
qryList.sql.Clear;
qryList.SQL.Add(' Select distinct ');
qryList.SQL.Add(' A.grade ');
qryList.SQL.Add(' , A.code ');
qryList.SQL.Add(' , A.description');
qryList.SQL.Add(' FROM ESCW019 A ');
qryList.SQL.Add(' WHERE A.year = :year');
qryList.ParamByName('year').AsString := year;
qryList.Open;
html := '';
html := html + '<select class="comboselect2" id="'+idComboBox+'" style="width:100%" '+enabled+' onchange='+action+'>' + SLineBreak;
if key = '' then
html := html + ' <option value="" selected="selected">Select</option>' + SLineBreak
else
html := html + ' <option value="" >Select</option>' + SLineBreak;
qryList.First;
while not qryList.Eof do
begin
if (key <> '') and (qryList.FieldByName('code').AsString = key) then
html := html + ' <option value="'+qryList.FieldByName('code').AsString+'" selected="selected">'+qryList.FieldByName('desc').AsString+'</option> ' + SLineBreak
else
html := html + ' <option value="'+qryList.FieldByName('code').AsString+'">'+qryList.FieldByName('desc').AsString+'</option> ' + SLineBreak;
qryList.Next;
end;
html := html + '</select> ' + SLineBreak;
qryList.Close;
Result := html;
finally
qryList.Free;
end;
end;He asked me what I would consider to change in code like this.
Here's my code review rant list
1- There is no reason to use WideStrings. WideString should be used in COM and when calling Windows API directly. Other than that, no reason.
2- Setting the SQL: Each time qryList.SQL.Add() is called, a whole bunch of methods and events are triggered. Some events may even trigger Database requests (we never know). Don't do this. Set the SQL at once using a single Add() or, what I do, qryList.SQL.Text := 'bla bla bla'
3- String concatenation in a loop: Always a bad idea. You can get better performance using something like our TStrBuilder (declared in unit IW.Common.StrBuilder)
4- Using FieldByName() inside a tight loop. FieldByName performance has improved but it won't ever be as efficient as having the field reference to use directly. Use FieldByName() 1 time to get the field reference and then use it inside the loop.
5 - DataSet loop: As a rule of thumb, use DataSet.DisableControls..DataSet.EnableControls when looping a DataSet. You possibly don't know the implementation details of the DataSet. In this case you don't know if DisableControls will improve the performance or not. It does make a big difference in ADO DataSets, for instance.
6- One query field is not used at all
7 - Several statements have no reason to exist. Query.Close before destroying it, SQL.Clear just after creating the query, and various others.
Tomorrow I'll publish the code I wrote to replace this, keeping the same functionality and method signature.