| Welcome, Guest |
You have to register before you can post on our site.
|
| Latest Threads |
WebApplication.IP change ...
Forum: IntraWeb General Discussion
Last Post: MJS@mjs.us
09-11-2026, 09:49 PM
» Replies: 1
» Views: 136
|
Hook/callback to handle t...
Forum: IntraWeb General Discussion
Last Post: Lenfors
09-08-2026, 09:03 AM
» Replies: 0
» Views: 124
|
Reproducible IIS crash in...
Forum: IntraWeb General Discussion
Last Post: alex.trejo@tttnet.com.mx
09-08-2026, 01:48 AM
» Replies: 0
» Views: 141
|
The packages IW16.xx inc...
Forum: IntraWeb General Discussion
Last Post: hsbelli
09-03-2026, 11:46 AM
» Replies: 2
» Views: 431
|
Redirection issues with F...
Forum: IntraWeb General Discussion
Last Post: magosk
08-31-2026, 10:03 AM
» Replies: 0
» Views: 216
|
Source Code
Forum: IntraWeb General Discussion
Last Post: magosk
08-31-2026, 08:11 AM
» Replies: 3
» Views: 605
|
TContenthandler requires ...
Forum: IntraWeb General Discussion
Last Post: valmeras
08-30-2026, 02:35 AM
» Replies: 0
» Views: 207
|
TIWjQDBGrid erratic behav...
Forum: IntraWeb General Discussion
Last Post: alex.trejo@tttnet.com.mx
08-20-2026, 05:12 PM
» Replies: 8
» Views: 2,185
|
Change Language DataTable
Forum: IntraWeb General Discussion
Last Post: Rigoberto Mercedes
08-19-2026, 03:21 PM
» Replies: 1
» Views: 338
|
How to terminate HTTP Thr...
Forum: Indy
Last Post: rlebeau
08-14-2026, 08:01 AM
» Replies: 5
» Views: 949
|
|
|
| Refactoring old code |
|
Posted by: Alexandre Machado - 07-18-2023, 05:54 AM - Forum: IntraWeb General Discussion
- Replies (2)
|
 |
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):
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.
|
|
|
|
).![[Image: empty_modal_za86nw.png]](https://res.cloudinary.com/teka/image/upload/v1688566217/empty_modal_za86nw.png)
![[Image: content_modal_ylnq58.png]](https://res.cloudinary.com/teka/image/upload/v1688566683/content_modal_ylnq58.png)
![[Image: expected_modal_ptgijh.png]](https://res.cloudinary.com/teka/image/upload/v1688565338/expected_modal_ptgijh.png)
