Refactoring old code

[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.