How to refactor copying/moving data between arrays?

Question

I have a process that reads data into 150+ temp arrays, process the data and copies from temp arrays into work arrays. Work arrays are in one global array so I can import multiple data, means I can repeat the same process up to 100 times and end up with set of on big array that holds 100x work data I can work with, compare and do stuff.

I have 150+ arrays, so 150 times:

    // for each array
    SetLength(myData[Idx].WorkNames,Length(tmpNames)); // <- prepare to copy
    for i := 0 to High(tmpNames) do  // <- copy
      myData[Idx].WorkNames[i]:=tmpNames[i];
    SetLength(tmpNames,0);  //  <- clear tmp array

4 lines of code for each array - 150x4 = 600 loc + initial + empty lines - around 900 loc.

Here is the example of what I do:

type

  TName = record
    NameID:integer;
    Description:string;
  end;

  TItem = record
    ItemID:integer;
    Description:string;
    Active:boolean;
  end;

  TWorkData = record
      WorkDataType:string;
      WorkNames:array of TName;
      WorkItems:array of TItem;
    end;

var

  AllWorkData:array of TWorkData;  //  <- global array that has all work data - up to 100x sets of work data
  tmpNames:array of TName; // <- tmp arrays before saving to work array
  tmpItems:array of TItem; // 

procedure TForm1.Button1Click(Sender: TObject);
var i,Idx:integer;
begin

  // 1. read data into tmp arrays
  ReadDataIntoTmpArrays;
  ProcessTmpData;

  // 2. copy tmp arrays into work data
  Idx:=GetWorkDataIdx; // <- work data sequence number; start with 0
  AllWorkData[Idx].WorkDataType:=GetWorkDataName(Idx);
  SetLength(AllWorkData[Idx].WorkNames,Length(tmpNames));
  SetLength(AllWorkData[Idx].WorkItems,Length(tmpItems));

  for i := 0 to High(tmpNames) do
    AllWorkData[Idx].WorkNames[i]:=tmpNames[i];

  for i := 0 to High(tmpItems) do
    AllWorkData[Idx].WorkItems[i]:=tmpItems[i];

  // 3. clear tmp arrays
  SetLength(tmpNames,0);
  SetLength(tmpItems,0);

end;

Question: Is there something I can do that is easier to maintain, refactor the code?

score 1 · Accepted Answer · edited May 23 '17 at 12:04

If you really do want to copy, then do so using generics. You could derive from the TArray class of static class methods declared in System.Generics.Collections. For instance:

type
  TArray = class(Generics.Collections.TArray)
  public
    class function Copy<T>(const Source: array of T; Index, Count: Integer): TArray<T>; overload; static;
    class function Copy<T>(const Source: array of T): TArray<T>; overload; static;
  end;

....

class function TArray.Copy<T>(const Source: array of T; Index, Count: Integer): TArray<T>;
var
  i: Integer;
begin
  SetLength(Result, Count);
  for i := 0 to high(Result) do begin
    Result[i] := Source[i+Index];
  end;
end;

class function TArray.Copy<T>(const Source: array of T): TArray<T>;
var
  i: Integer;
begin
  SetLength(Result, Length(Source));
  for i := 0 to high(Result) do begin
    Result[i] := Source[i];
  end;
end;

Note that all of the above requires you to stop using array of TMyType and instead start using the generic dynamic array TArray<TMyType>.

In your case though you are overcomplicating. Replace:

SetLength(myData[Idx].WorkNames,Length(tmpNames)); // <- prepare to copy
for i := 0 to High(tmpNames) do  // <- copy
  myData[Idx].WorkNames[i]:=tmpNames[i];
SetLength(tmpNames,0);  //  <- clear tmp array

with:

myData[Idx].WorkNames := tmpNames;
tmpNames := nil;

If you are prepared to let tmpNames simply leave scope then you can use a single line:

myData[Idx].WorkNames := tmpNames;

Although, if tmpNames is re-used for a different array, then the nil assignment is needed.

Then again, as far as I can see from the code in the question you don't need the temporary arrays at all. Why not operate directly on the long lived data structures.

These array assignments are only permissible if the source and target of the assignment are assignment compatible. Your types are not because you have used distinct types. Switch to TArray<T> to avoid that. See this question for more: Why are two seemingly identical dynamic array types deemed not assignment compatible?

Remember that dynamic arrays are reference types. In the usage shown here, all you need to do is copy the reference. You only need one instance of the actual array. So copying is not necessary at all.

This line `AllWorkData[0].WorkNames:=tmpNames;` says: `[dcc32 Error] Unit1.pas(90): E2008 Incompatible types`. What am i doing wrong? — Mike Torrettinni, Dec 26 '15 at 19:47
It's because .`WorkNames` and `tmpNames` are distinct types that are not assignment compatible. Use `TArray` instead, substituting the appropriate type for `T`. — David Heffernan, Dec 26 '15 at 20:02
Aha, now it works. So, you suggest replacing all `array of T` with `TArray`... — Mike Torrettinni, Dec 26 '15 at 20:49
Yes. Absolutely that is a good move. It will allow much greater flexibility and composability. — David Heffernan, Dec 26 '15 at 20:51
I'm just reading the documentation, SO and searching to find any performance issue/degradation when accessing/assigning values to TArray compared to old array type. Nothing found so far. The change is substantial, if I go into it, but I assume the benefits like in your answer might outweigh all else. I just can't ignore the feeling that TArray fires all sort of events when reading, assigning values - events that make it work so 'better' than old types. — Mike Torrettinni, Dec 26 '15 at 21:02
@MikeTorrettinni No. The emitted code is identical. They are still dynamic arrays. There are no events. — David Heffernan, Dec 26 '15 at 22:01

How to refactor copying/moving data between arrays?

1 Answers1