Should Exit be used instead of 'record Exists?' boolean?

[Mike Torrettinni 198]

This is probably very subjective, but I'm just trying to get another view/thoughts on this example:

 

when adding simple records (not a lot of lines of code), I used to always use 1st example, DataSave1: use boolean flag and add new if record not exsits, but lately I try using Exit, like in 2nd example, DataSave2: - update record and Exit, flow never gets to adding new record:

 

procedure SaveData1(aID: Integer; const aName: string);
var
  i: Integer;
  vExists: boolean;
  vNewData: TDataRec;
begin
  vExists := false;
  for i := Low(Data) to High(Data) do
  if Data[i].DataID = aID then
  begin
    Data[i].DataName := aName;
    vExists := True;
    Break;
  end;

  // Save new Data rec, if ID not found
  if Not vExists then
  begin
    vNewData := Default(TDataRec);
    vNewData.DataID := aID;
    vNewData.DataName := aName;
    Data := Data + [vNewData];
  end;
end;

or without boolean flag:

 

procedure SaveData2(aID: Integer; const aName: string);
var
  i: Integer;
  vNewData: TDataRec;
begin
  for i := Low(Data) to High(Data) do
  if Data[i].DataID = aID then
  begin
    Data[i].DataName := aName;
    Exit;
  end;

  // Save new Data rec, if ID not found
  vNewData := Default(TDataRec);
  vNewData.DataID := aID;
  vNewData.DataName := aName;
  Data := Data + [vNewData];
end;

I like 2nd example because I don't need vExists flag. Am I setting myself up to some unknown issue that I can't see?

 

These are examples for small Data arrays.

type
  TDataRec = record
    DataID: integer;
    DataName: string;
  end;
var
  Data: TArray<TDataRec>;

 

Edited September 9, 2020 by Mike Torrettinni

[Stefan Glienke 2002]

I would use the Exit version and put the appending new record into its own method.

[dummzeuch 1505]

Personally I see nothing wrong with using exit there. I usually add a comment to it to make it more visible.

procedure SaveData2(aID: Integer; const aName: string);
var
  i: Integer;
  vNewData: TDataRec;
begin
  for i := Low(Data) to High(Data) do
  if Data[i].DataID = aID then
  begin
    Data[i].DataName := aName;
    Exit; //==>
  end;

  // Save new Data rec, if ID not found
  // ...
end;

(But people have criticized me for adding that comment.)

 

Of course purists will tell you that exit is just a goto in disguise and that "everybody knows that "goto is considered harmful".

[Mike Torrettinni 198]

41 minutes ago, Stefan Glienke said:

I would use the Exit version and put the appending new record into its own method.

Great, thanks!

I didn't think about this at all. I was thinking somebody will come up with some trick how to do complicated if else for this, but this is smarter solution.

[Mike Torrettinni 198]

10 minutes ago, dummzeuch said:

Of course purists will tell you that exit is just a goto in disguise and that "everybody knows that "goto is considered harmful".

I use Exit a lot as 'early exit' on conditions, but those are usually simple and very obvious statements at the beginning of the methods.

 

Not sure in which version it was introduced, but I like how Exit is marked with a red arrow (you can't miss it):

 

Edited September 9, 2020 by Mike Torrettinni

[Lars Fosdal 1792]

How many elements in Data ?

[Mike Torrettinni 198]

5 minutes ago, Lars Fosdal said:

How many elements in Data ?

Different record structures, from a few to nested records, why is this important?

Hm, actually not sure are you asking about how many records in Data or elements in TDataRec?

Edited September 9, 2020 by Mike Torrettinni

[Bill Meyer 337]

30 minutes ago, dummzeuch said:

Of course purists will tell you that exit is just a goto in disguise and that "everybody knows that "goto is considered harmful".

But then, any high level language is just a collection of GOTOs in disguise. Any instance of a jump instruction in the machine code is a GOTO. Look in the CPU window, and you will see jmp and jz in many places. Moreover, Break and Continue are disguised GOTOs just as much as is Exit.

There are good reasons to use GOTO, as has been pointed out by Knuth and others.

 

Edited September 9, 2020 by Bill Meyer

[FPiette 383]

I always use the Exit method.

 

[Remy Lebeau 1394]

2 hours ago, Mike Torrettinni said:

I didn't think about this at all. I was thinking somebody will come up with some trick how to do complicated if else for this, but this is smarter solution.

Just my 2-cents worth - I prefer to use something more like this:

type
  PDataRec = ^TDataRec
  TDataRec = record
    DataID: integer;
    DataName: string;
  end;

var
  Data: TArray<TDataRec>;

function FindRecord(aID: Integer): PDataRec;
var
  i: Integer;
begin
  Result := nil;
  for i := Low(Data) to High(Data) do
  begin
    if Data[i].DataID = aID then
    begin
      Result := @Data[i];
      Exit;
    end;
  end;
end;

function AddRecord(aID: Integer): PDataRec;
var
  vNewData: TDataRec;
begin
  vNewData := Default(TDataRec);
  vNewData.DataID := aID;
  Data := Data + [vNewData];
  Result := @Data[High(Data)];
end;

function EnsureRecord(aID: Integer): PDataRec;
begin
  Result := FindRecord(aID);
  if Result = nil then
    Result := AddRecord(aID);
end;

procedure SaveData1(aID: Integer; const aName: string);
var
  pData: PDataRec;
begin
  pData := EnsureRecord(aID);
  pData.DataName := aName;
end;

 

[Guest]

2 hours ago, Bill Meyer said:

But then, any high level language is just a collection of GOTOs in disguise. Any instance of a jump instruction in the machine code is a GOTO. Look in the CPU window, and you will see jmp and jz in many places. Moreover, Break and Continue are disguised GOTOs just as much as is Exit.

Yes! Google "Turingism" or "Turing Machine". Spot on! The government-killed genius of WW2 defined this better than his predecessors.

2 hours ago, Bill Meyer said:

There are good reasons to use GOTO, as has been pointed out by Knuth and others.

Ehrm... IMHO; nope. The TM's third operation is better concealed in high-level patterns IMHO.

[Bill Meyer 337]

 

3 minutes ago, Dany Marmur said:

Ehrm... IMHO; nope. The TM's third operation is better concealed in high-level patterns IMHO.

I'll go with Knuth's opinion. 😉

[Mike Torrettinni 198]

1 hour ago, Remy Lebeau said:

Just my 2-cents worth - I prefer to use something more like this:

type
  PDataRec = ^TDataRec
  TDataRec = record
    DataID: integer;
    DataName: string;
  end;

var
  Data: TArray<TDataRec>;

function FindRecord(aID: Integer): PDataRec;
var
  i: Integer;
begin
  Result := nil;
  for i := Low(Data) to High(Data) do
  begin
    if Data[i].DataID = aID then
    begin
      Result := @Data[i];
      Exit;
    end;
  end;
end;

function AddRecord(aID: Integer): PDataRec;
var
  vNewData: TDataRec;
begin
  vNewData := Default(TDataRec);
  vNewData.DataID := aID;
  Data := Data + [vNewData];
  Result := @Data[High(Data)];
end;

function EnsureRecord(aID: Integer): PDataRec;
begin
  Result := FindRecord(aID);
  if Result = nil then
    Result := AddRecord(aID);
end;

procedure SaveData1(aID: Integer; const aName: string);
var
  pData: PDataRec;
begin
  pData := EnsureRecord(aID);
  pData.DataName := aName;
end;

 

Seems pretty useful!

In case a record has 2 (or more) unique identifiers... like ID and ProjectId, would you then have: EnsureRecord(ProjectID, ID) ? Or I misunderstood the concept?

[Guest]

In many cases of loops in general and almost always with the case of finding an element, i use the same as Remy with few small differences, the FindRecord will be FindRecordByID and the result of that is an index or -1, this makes all its usage simple as loading the result in a local var when the index will be needed or directly called from the if statement, also will simplify its usage to something like if .. then or if .. then .. else, and of course you can leave it as class function, global function or a helper for your data type.

[Mike Torrettinni 198]

9 minutes ago, Kas Ob. said:

In many cases of loops in general and almost always with the case of finding an element, i use the same as Remy with few small differences, the FindRecord will be FindRecordByID and the result of that is an index or -1, this makes all its usage simple as loading the result in a local var when the index will be needed or directly called from the if statement, also will simplify its usage to something like if .. then or if .. then .. else, and of course you can leave it as class function, global function or a helper for your data type.

You packed a lot of information into this and I'm a little lost, could you just show example how you set up your  if .. then or if .. then .. else?

[Guest]

function FindRecordByID(aID: Integer): Integer;
var
  i: Integer;
begin
  Result := -1;
  for i := Low(Data) to High(Data) do
    if Data[i].DataID = aID then
    begin
      Result := i;
      break;
    end;
end;

if .. then .. else

procedure SaveData1(aID: Integer; const aName: string);
var
  i: Integer;
  vNewData: TDataRec;
begin
  i := FindRecordByID(aID);

  if i <> -1 then
  begin
    Data[i].DataName := aName;
  end else
  begin
    vNewData := Default(TDataRec);
    vNewData.DataID := aID;
    vNewData.DataName := aName;
    Data := Data + [vNewData];
  end;
end;

if .. then

procedure SaveData2(aID: Integer; const aName: string);
var
  i: Integer;
  vNewData: TDataRec;
begin
  i := FindRecordByID(aID);
  if i > -1 then
  begin
    Data[i].DataName := aName;
    exit;
  end;

  vNewData := Default(TDataRec);
  vNewData.DataID := aID;
  vNewData.DataName := aName;
  Data := Data + [vNewData];
end;

Another case if..then when the index is not needed

procedure AddIfNotExist(aID: Integer; const aName: string);
var
  vNewData: TDataRec;
begin
  if FindRecordByID(aID) = -1 then
  begin
    vNewData := Default(TDataRec);
    vNewData.DataID := aID;
    vNewData.DataName := aName;
    Data := Data + [vNewData];
  end;
end;

 

[Remy Lebeau 1394]

2 hours ago, Mike Torrettinni said:

In case a record has 2 (or more) unique identifiers... like ID and ProjectId, would you then have: EnsureRecord(ProjectID, ID) ?

Yes.  Though, in the case of multiple identifiers, it may make sense to wrap them in another record, if that suits your needs, eg:

type
  TDataRecID = record
    ProjectID: Integer; // or wharever
    ID: integer;
  end;

  PDataRec = ^TDataRec
  TDataRec = record
    DataID: TDataRecID;
    DataName: string;
  end;

var
  Data: TArray<TDataRec>;

function FindRecord(const aID: TDataRecID): PDataRec;
var
  i: Integer;
begin
  Result := nil;
  for i := Low(Data) to High(Data) do
  begin
    if (Data[i].DataID.ProjectID = aID.ProjectID) and
       (Data[i].DataID.ID = aID.ID) then
    begin
      Result := @Data[i];
      Exit;
    end;
  end;
end;

function AddRecord(const aID: TDataRecID): PDataRec;
var
  vNewData: TDataRec;
begin
  vNewData := Default(TDataRec);
  vNewData.DataID := aID;
  Data := Data + [vNewData];
  Result := @Data[High(Data)];
end;

function EnsureRecord(const aID: TDataRecID): PDataRec;
begin
  Result := FindRecord(aID);
  if Result = nil then
    Result := AddRecord(aID);
end;

procedure SaveData1(const aID: TDataRecID; const aName: string);
var
  pData: PDataRec;
begin
  pData := EnsureRecord(aID);
  pData.DataName := aName;
end;

Otherwise, yes, you could just handle them individually:

type
  PDataRec = ^TDataRec
  TDataRec = record
    DataID: Integer;
    DataName: string;
    ProjectID: Integer; // or whatever
  end;

var
  Data: TArray<TDataRec>;

function FindRecord(aProjectID, aID: TDataRecID): PDataRec;
var
  i: Integer;
begin
  Result := nil;
  for i := Low(Data) to High(Data) do
  begin
    if (Data[i].ProjectID = aProjectID) and
       (Data[i].DataID = aID) then
    begin
      Result := @Data[i];
      Exit;
    end;
  end;
end;

function AddRecord(aProjectID, aID: Integer): PDataRec;
var
  vNewData: TDataRec;
begin
  vNewData := Default(TDataRec);
  vNewData.DataID := aID;
  vNewData.ProjectID := aProjectID;
  Data := Data + [vNewData];
  Result := @Data[High(Data)];
end;

function EnsureRecord(aProjectID, aID: Integer): PDataRec;
begin
  Result := FindRecord(aProjectID, aID);
  if Result = nil then
    Result := AddRecord(aProjectID, aID);
end;

procedure SaveData1(aProjectID, aID: Integer; const aName: string);
var
  pData: PDataRec;
begin
  pData := EnsureRecord(aProjectID, aID);
  pData.DataName := aName;
end;

 

Edited September 9, 2020 by Remy Lebeau

[Mike Torrettinni 198]

Great, thank you for all the suggestions! Got a lot to think about.

[Fr0sT.Brutal 900]

In my practice if I made Find* methods first, later I always came to situation when I also needed the index of the item found. So I had to add IndexOf method and then use it in Find* methods. Why not doing this from the start 🙂

[Mike Torrettinni 198]

One last follow up question: do you add these IndexOf*, Find*, AddNew*... methods on all records or just important arrays, lists? I assume you don't make these methods for temporary or records not exposed to outside of a feature or unit, right?

[dummzeuch 1505]

Regarding Find-methods: I always make them return a boolean. If I need the index too, I make that an additional out parameter. If it gets too much hassle to always pass a dummy variable, I overload that method with one that does it for me and declare it inline:

 

function TBla.Find(const _Key: TSomeType; out _Data: TSomeOtherType): Boolean; overload; inline;
var
  Idx: Integer;
begin
  Result := Find(_Key, _Data, Idx);
end;

 

Edited September 10, 2020 by dummzeuch

[Remy Lebeau 1394]

6 hours ago, Fr0sT.Brutal said:

In my practice if I made Find* methods first, later I always came to situation when I also needed the index of the item found. So I had to add IndexOf method and then use it in Find* methods. Why not doing this from the start 🙂

In cases where I need the index of a found item, I prefer to have my Find* functions accept an optional parameter to output the index.  That way, I can still return a pointer to the actual data and not have to re-index back into the data storage after Find* exits, but I can still use the index for other things as needed.  For example:

 

function FindRecord(aID: Integer; vIndex: PInteger = nil): PDataRec;
var
  i: Integer;
begin
  Result := nil;
  if vIndex <> nil then vIndex^ := -1;
  for i := Low(Data) to High(Data) do
  begin
    if Data[i].DataID = aID then
    begin
      Result := @Data[i];
      if vIndex <> nil then vIndex^ := i;
      Exit;
    end;
  end;
end;

Or:

function FindRecord(aID: Integer; var vIndex: Integer): PDataRec; overload;
var
  i: Integer;
begin
  Result := nil;
  vIndex := -1;
  for i := Low(Data) to High(Data) do
  begin
    if Data[i].DataID = aID then
    begin
      Result := @Data[i];
      vIndex := i;
      Exit;
    end;
  end;
end;

function FindRecord(aID: Integer): PDataRec; overload;
var
  ignore: Integer;
begin
  Result := FindRecord(aProjectID, aID, ignore);
end;

 

Edited September 10, 2020 by Remy Lebeau

[Fr0sT.Brutal 900]

4 hours ago, Mike Torrettinni said:

One last follow up question: do you add these IndexOf*, Find*, AddNew*... methods on all records or just important arrays, lists? I assume you don't make these methods for temporary or records not exposed to outside of a feature or unit, right?

As for me, everywhere I need search in arrays, I use generic classes or wrappers. Of course, if I have an array and need finding an item in just one place, I don't implement separate functions

@dummzeuch, @Remy Lebeau

good point though I prefer separating the methods

[bobD 6]

On 9/9/2020 at 10:43 AM, Remy Lebeau said:

Just my 2-cents worth - I prefer to use something more like this:

What Remy's approach does is accomplish OO 'separation of concerns'--decomposing all the logic of 'Save' into those component parts which might be subject to independent change and testing. So while a bit more code initially, it's much more maintainable and reusable. The pattern is equally applicable whether the storage mechanism is a TList or a zillion record database--only the technical details of how find, inset, and update work need be addressed. The higher level application logic is stable because all the underlying technical details have been isolated out.

 bobD

[microtronx 38]

I would use "Exit;". 

 

Having nearly same needs and have a created a tObject for managing a tDictionary<string, __myrecord> to manage everything ... very fast IMO.

Be sure to use "MonitorEnter" and MonitorExit if this can be used in multithreaded applications like

 

function tmyComponent.getvaluerecord(vName: string; const vDefaultValue: variant): __MyRecord;
var
    vValue:__MyRecord;
begin
    if MonitorEnter(fDATA, fMonitorTimeout) then begin
        try
            vname:=trim(lowercase(vname));

            if fData.TryGetValue(vname, result)=false then begin // we don't have that key in dictionary ... return default value ... or create a new entry etc..
                result.vwert:=vDefaultValue;
                result.isObject:=false;
                result.isFile:=false;
                result.doFreeObject:=false;
                result.vcomponent:=nil;
                result.vname:='';
                result.vdtstamp:=0;

                result.vtype:='';
                result.vgroup:='';
            end;
        finally
            MonitorExit(fDATA);
        end;
    end;
end;