Refactoring old code

[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):

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.

[Alexandre Machado]

As promised, here is the refactored code (untested, but should be good :-) ):

Code:

uses
  IW.Common.StrBuilder;
 
function CreateSelectHtml(const idComboBox, key, year, enabled, action: string): string;
var   
  qryList: TFDQuery;
  sb: IStrBuilder;
  fldCode,
  fldDesc: TField;
begin
  Result := '';
  qryList := TFDQuery.Create(nil);
  try
    qryList.Connection := Controller.Connection;
    qryList.SQL := ' Select distinct A.code, A.description ' +
           ' FROM ESCW019 A ' +
           ' WHERE A.year = :year ';
    qryList.ParamByName('year').AsString := year;
    qryList.Open;

    TStrBuilder.GetInstance(sb);
    
    sb.Add('<select class="comboselect2"  id="', idComboBox, '" style="width:100%" ', enabled, ' onchange=', action, '>', SLineBreak);
    if key = '' then
      sb.Add('   <option value="" selected="selected">Select</option>', SLineBreak)
    else
      sb.Add('   <option value="" >Select</option>', SLineBreak);

    fldCode := qryList.FieldByName('code');
    fldDesc := qryList.FieldByName('description');
 
    qryList.DisableControls;
    try
      while not qryList.Eof do
      begin
        if (key <> '') and (fldDesc.AsString = key) then
          sb.Add('        <option value="', fldCode.AsString, '" selected="selected">', fldDesc.AsString, '</option> ', SLineBreak)
        else
          sb.Add('        <option value="', fldCode, '">', fldDesc.AsString, '</option> ', SLineBreak);
        qryList.Next;
      end;
      sb.Add('</select> ', SLineBreak);
      qryList.Close;
    end;
    
    Result := sb.ToString;
  finally
    qryList.Free;
  end;
end;

While I was refactoring it, found some other minor and annoying issues that wouldn't pass my code review (like all var string parameters when nothing is changed internally, etc). Anyway, the refactored code performs better, has no useless code (apart from setting the result to an empty string on the top of the method that may trigger a compiler hint in modern Delphi compilers and should be removed in that case).

As mentioned in the previous post, the fields references are obtained once and used within the loop. This severely reduces the loop execution time if you have many records.

The other main change here is that we are now using the IStrBuilder interface (implemented by a TStrBuilder object) instead of string concatenation. The IStrBuilder has an interesting feature here: When the interface reference is obtained with via TStrBuilder.GetInstance() class method, it will obtain the reference from a pool of TStrBuilder objects. 

There are 2 main advantages in using it as we did: 
1) The object doesn't need to be created/destroyed because it is retrieved from/returned to the pool automatically so there is no overhead in using it.
2) There is no need to use a try..finally to protect the IStrBuilder usage because it will be handled accordingly (returned to the pool) as soon as it goes out of scope (when the method terminates)

The TStrBuilder performs better than the string concatenation. But the problem of simple string concatenation on multi-threaded servers is not the performance per se: Whenever you concatenate 2 strings, the compiler injects some LOCK asm instructions in the middle of the generated code. Long story short: LOCK instructions may hurt the overall performance of the processor (even though you don't feel any performance impact on that specific thread).

If you are curious about this LOCK-ing mechanism, there are a few pages out there where well known Delphi developers investigate it. In a nutshell, this is a good reference:

https://stackoverflow.com/questions/3339...-core-cpus

You can safely use the same pattern above in your own code that concatenate strings.

[Alexandre Machado]

I got some questions regarding the IStrBuilder, and the underlying TStrBuilder class, more specifically why it doesn't implement a fluent interface which would allow method chaining.

In that case, method Add() should return an IStrBuilder interface allowing code such as: sb.Add('something').Add('something else').Add('bla bla bla').Add('you got the idea').

It doesn't implement that pattern for 2 main reasons:

1- In Delphi, functions that return an interface are slower than a procedure that doesn't return anything. "Oh but it doesn't make any real difference". Yes it does. The implementation has the tough task to beat pure string concatenation in Delphi which is already very fast and efficient, so anything that can be done to make the code more efficient counts. That's also why the GetInstance() method uses a var parameter, instead of a function. According to our benchmark, using our StrBuilder makes the string concatenation code 40% faster in a single thread, and this margin increases steadily when the number of threads increases.

2- Although I like fluent interfaces and I believe that some code can benefit from it, developers tend to abuse it and create code that just can't be maintained. This is how most JavaScript is written nowadays. This is a real nightmare to read and especially to maintain (i.e. refactor and/or change when needed).

   

Not convinced of how bad it can be yet? Let's see this other example extracted from a real world C# code base where Automapper library is used (warning, this can't be unseen). BTW, this method chaining expands for exact 108 lines of code, I just extracted 1/3 of it:

Code:

protected override void Map()
        {
            Config = new MapperConfiguration(conf =>
            {
                conf.CreateMap<FileAppSettings, AppSettingsViewModel>()
                    .ForMember(target => target.NumberSelected,
                        opt =>
                            opt.MapFrom(
                                source =>
                                    PopulateSource(source.NumberSelected, Global.LabelList, false,
                                        SourceListNameEnum.Field_List).ToObservableCollection()))
                    .ForMember(target => target.ComponentSelected,
                        opt =>
                            opt.MapFrom(
                                source =>
                                    PopulateSource(source.ComponentSelected, Global.LabelList, false,
                                        SourceListNameEnum.Field_List).ToObservableCollection()))
                    .ForMember(target => target.DescSelected,
                        opt =>
                            opt.MapFrom(
                                source =>
                                    PopulateSource(source.DescSelected, Global.LabelList, false,
                                        SourceListNameEnum.Field_List).ToObservableCollection()))
                    .ForMember(target => target.InfoSelected,
                        opt =>
                            opt.MapFrom(
                                source =>
                                    PopulateSource(source.InfoSelected, Global.LabelList, true,
                                        SourceListNameEnum.InfoField_List).ToObservableCollection()))
                    .ForMember(target => target.SpecificFieldList,
                        opt =>
                            opt.MapFrom(
                                source =>
                                    FilterList(source.SpecificFieldList.ToObservableCollection(),
                                        source.NumberSelected,
                                        source.ComponentSelected,
                                        source.DescSelected, false)))
.......

So, in the end, although fluent interface and method chaining were actually good ideas, like everything else, developers found a way to ruin it 

And that's why we don't use it ourselves for our TStrBuilder.