Delphi - access class string property by its value

Question

I have a class defined which contains only strings as properties, and I need to get the property name based on its value as in the example below. In the example there are only 3 properties, in the real life class there are almost 1000. The problem is that this class is heavily used, and I want to know if I can get the property name by its value in a faster way.

unit Unit5;

interface

uses
  Windows, Messages, SysUtils, Variants, Classes, Graphics, Controls, Forms,
  Dialogs,RTTI, StdCtrls, Diagnostics;

type
  TConstDBElem = class
  public
    CCFN_1 : String;
    CCFN_2 : String;
    CCFN_3 : String;
    constructor Create;
  end;

  TForm5 = class(TForm)
    Memo1: TMemo;
    Button1: TButton;
    procedure Button1Click(Sender: TObject);
  end;

var
  Form5: TForm5;
  Obj: TConstDBElem;

implementation


{$R *.dfm}

procedure TForm5.Button1Click(Sender: TObject);
var iPos:Integer;
    timer:TStopwatch;
    Function GetName(const DBElemInstance : TConstDBElem; valueName: string) : string;
  var
    vrttiContext: TRttiContext;
    vrttiField : TRttiField;
    vType : TRttiType;
  begin
      vType := vrttiContext.GetType(TConstDBElem);

      for vrttiField in vType.GetFields do
        if (vrttiField.GetValue(DBElemInstance).ToString = valueName) then
        begin
           result := vrttiField.Name;
         end;
  end;

begin
  timer := TStopwatch.Create;
  timer.Start;
  Memo1.Lines.Clear;
  for iPos := 0 to 100000 do
    GetName(Obj,'TEST3');
  timer.Stop;
  Memo1.Lines.Add(FloatToStr(timer.Elapsed.TotalSeconds));
end;

constructor TConstDBElem.Create;
begin
  CCFN_1 := 'TEST1';
  CCFN_2 := 'TEST2';
  CCFN_3 := 'TEST3' ;
end;

initialization
  Obj := TConstDBElem.Create;

finalization
  Obj.Free;


end.

Yes,I know this is a very bad design, and this should not be done like this. Is there any option to make this search faster?

Answer 1

When GetName() finds a match, it is not stopping its loop, so it keeps searching for more matches. Assigning a function's Result does not exit the function, like you clearly think it does. As such, GetName() ends up returning the last match, not the first match. The loop should be calling Exit when it finds the first match:

Function GetName(const DBElemInstance : TConstDBElem; valueName: string) : string;
var
  vrttiContext: TRttiContext;
  vrttiField : TRttiField;
  vType : TRttiType;
begin
  vType := vrttiContext.GetType(TConstDBElem);

  for vrttiField in vType.GetFields do
    if (vrttiField.GetValue(DBElemInstance).ToString = valueName) then
    begin
      result := vrttiField.Name;
      Exit; // <-- add this
    end;
end;

Alternatively, use the version of Exit() that takes a parameter:

Function GetName(const DBElemInstance : TConstDBElem; valueName: string) : string;
var
  vrttiContext: TRttiContext;
  vrttiField : TRttiField;
  vType : TRttiType;
begin
  vType := vrttiContext.GetType(TConstDBElem);

  for vrttiField in vType.GetFields do
    if (vrttiField.GetValue(DBElemInstance).ToString = valueName) then
    begin
      Exit(vrttiField.Name); // <-- assigns Result and exits at the same time
    end;
end;

In your simple example, the time wasted to search 3 fields is hardly noticeable, but when searching 1000 fields, it makes a difference.

Answer 2

You state in a comment that the values never change at runtime. In which case you can simply build a single dictionary at startup that has the property values as the dictionary key, and the property name as the dictionary value.

I'm assuming that all instances of the class have the same property values. If that's not the case then you'll need one dictionary per instance.

Answer 3

It's a "bad design" because someone wrote a class that they're treating like a C-style Struct. As has been said already, there are NO PROPERTIES defined in the class, just a bunch of PUBLIC DATA MEMBERS, aka, "fields".

There's no encapsulation, so anything you do to change the structure could have far-reaching implications on any unit that uses this. I agree that replacing the IMPLEMENTATION with a TStringList or a TDictionary would be smart, but ... there are no interfaces to adhere to! You have 1000-odd hard-wired references to public data members.

(The last time I saw something like this was some code written by a bunch of VB programmers who wrote classes as if they were C-style structs containing public data members, and then they wrote external functions to access the data, just as you'd do in C. Only they buried business logic inside of the accessor methods as well, which causes random pieces of the code to make direct references to the data members of the class.)

Off-hand, I'd say you're totally mis-using the RTTI code. Sure, the optimizations above will improve performance, but so what? It's the wrong solution!

If you really want to refactor this (and you certainly should!), first I'd look to see how widespread the use of this poor class is by changing the public to private and see how many errors you get.

Then I'd derive it from TStringList, delete all of the local fields, and move the GetName function inside of the class:

type
  TConstDBElem = class( TStringList )
  public
    constructor Create;
    function GetName( aName : string ) : string;
  end;

Now, if I'm interpreting your example correctly, you want to do this to initialize the object:

constructor TConstDBElem.Create;
begin
  Add( 'TEST1=CCFN_1' );
  Add( 'TEST2=CCFN_2' );
  Add( 'TEST3=CCFN_3' );
end;

Then replace all of the references in other units with a call to obj.GetName():

function TConstDBElem.GetName( aName : string ) : string;
begin
  Result := Values[aName];
end;

You're replacing a reference to obj.CCFN_1 (?) or GetName(obj,'TEST1') with obj.GetName('TEST1').

(Maybe I'm totally off-base at this point. Sorry, but I just don't get how you're using this class from the example, and it doesn't make a whole lot of sense to me anyway. It would make more sense if you said what you're mapping between. I mean ... who needs to look up a local field name from a value associated with it? And what do you do with it once you've found it? Whomever wrote this had to go through some incredible contortions to make the code work because s/he sure didn't understand OOP when this was designed!)

At this point, you will have succeeded in decoupling the clients of this class (other units) from its internal implementation, and replacing those references with calls to an interface (a method) defined in the class instead.

Then you can do some testing to see what happens if you change the implementation, like from a TStringList to a TDictionary. But no matter how you slice it, I cannot imagine that either the TStringList or TDictionary will be slower than how you're abusing the RTTI system in your example. :)