How to make this code re-usable

Question

I have a specific way to build nodes for Virtual Treeview (I douns this example years ago and never had a reason to change it, until now). As I use almost identical code in around 150 cases, I would like to try and get it re-usable and reduce overall code lines.

I'm attaching full code of example with 2 buttons and a Vritual Treeview on the form:

unit Unit1;

interface

uses
  Winapi.Windows, Winapi.Messages, System.SysUtils, System.Variants, System.Classes, Vcl.Graphics,
  Vcl.Controls, Vcl.Forms, Vcl.Dialogs, VirtualTrees, Vcl.StdCtrls;

type

  rTreeData = record
    IndexInMyData: integer;
  end;

  tLine = record
    Level:integer;
    Txt:string;
    NodePointer: PVirtualNode;
  end;

  TForm1 = class(TForm)
    Button1: TButton;
    VTV: TVirtualStringTree;
    Button2: TButton;
    procedure Button1Click(Sender: TObject);
    procedure Button2Click(Sender: TObject);
  private
    { Private declarations }
  public
    { Public declarations }
  end;

var
  Form1: TForm1;
  vArray:array of tLine;

implementation

{$R *.dfm}



procedure TForm1.Button1Click(Sender: TObject);
var
  Node: PVirtualNode;
  Data: ^rTreeData;
  i, j: integer;
begin
  SetLength(vArray,5);
  vArray[0].Level:=0; vArray[0].Txt:='One';
  vArray[1].Level:=1; vArray[1].Txt:='Two';
  vArray[2].Level:=1; vArray[2].Txt:='three';
  vArray[3].Level:=2; vArray[3].Txt:='Four';
  vArray[4].Level:=0; vArray[4].Txt:='Give';

  VTV.BeginUpdate;
  VTV.Clear;
  for i := Low(vArray) to High(vArray) do
  begin
     if i = 0 then
    begin
      Node := VTV.AddChild(nil);
      Data := VTV.GetNodeData(Node);
    end
    else
    begin
      if vArray[i].Level = 0 then
        Node := VTV.AddChild(nil)
      else if vArray[i].Level > vArray[i - 1].Level then
        Node := VTV.AddChild(Node)
      else if vArray[i].Level < vArray[i - 1].Level then
      begin
        Node := Node.Parent;
        for j := 1 to (vArray[i - 1].Level - vArray[i].Level) do
          Node := Node.Parent;
        Node := VTV.AddChild(Node);
      end
      else
      begin
        Node := Node.Parent;
        Node := VTV.AddChild(Node);
      end;

      Data := VTV.GetNodeData(Node);
    end;

    // Create link to your data record into VST node
    Data.IndexInMyData := i;
    vArray[Data.IndexInMyData].NodePointer := Node;
  end;
  VTV.FullExpand;
  VTV.EndUpdate;
end;

function AddNode(vTV: TvirtualStringTree; vI, vLevel, vLevelPrev:integer; vNode:PVirtualNode; var vData:rTreeData):PVirtualNode;
var j:integer;
begin
   if vI = 0 then
    begin
      Result := vTV.AddChild(nil);
      vData := rTreeData(vTV.GetNodeData(Result)^);
    end
    else
    begin
      if vLevel = 0 then  Result := vTV.AddChild(nil)
      else if vLevel > vLevelPrev then  Result := vTV.AddChild(vNode)
      else if vLevel < vLevelPrev then
      begin
        Result := vNode.Parent;
        for j := 1 to (vLevelPrev - vLevel) do
          Result := Result.Parent;
        Result := vTV.AddChild(Result);
      end
      else
      begin
        Result := vNode.Parent;
        Result := vTV.AddChild(Result);
      end;

      vData := rTreeData(vTV.GetNodeData(Result)^);
    end;
    vData.IndexInMyData := vI;
end;

procedure TForm1.Button2Click(Sender: TObject);
var
  Node: PVirtualNode;
  Data: ^rTreeData;

  i, j,vLevelPrev: integer;
begin
  SetLength(vArray,5);
  vArray[0].Level:=0; vArray[0].Txt:='One';
  vArray[1].Level:=1; vArray[1].Txt:='Two';
  vArray[2].Level:=1; vArray[2].Txt:='three';
  vArray[3].Level:=2; vArray[3].Txt:='Four';
  vArray[4].Level:=0; vArray[4].Txt:='Give';

  VTV.BeginUpdate;
  VTV.Clear;
  for i := Low(vArray) to High(vArray) do
  begin
    if i = 0 then
      vLevelPrev:=0
    else
      vLevelPrev:=vArray[i-1].Level;

      Node:=AddNode(VTV,i,vArray[i].Level,vLevelPrev,Node,rTreeData(Data^));

//     if i = 0 then
//    begin
//      Node := VTV.AddChild(nil);
//      Data := VTV.GetNodeData(Node);
//    end
//    else
//    begin
//      if vArray[i].Level = 0 then
//        Node := VTV.AddChild(nil)
//      else if vArray[i].Level > vArray[i - 1].Level then
//        Node := VTV.AddChild(Node)
//      else if vArray[i].Level < vArray[i - 1].Level then
//      begin
//        Node := Node.Parent;
//        for j := 1 to (vArray[i - 1].Level - vArray[i].Level) do
//          Node := Node.Parent;
//        Node := VTV.AddChild(Node);
//      end
//      else
//      begin
//        Node := Node.Parent;
//        Node := VTV.AddChild(Node);
//      end;
//
//      Data := VTV.GetNodeData(Node);
//    end;

    // Create link to your data record into VST node
    Data.IndexInMyData := i;
    vArray[Data.IndexInMyData].NodePointer := Node;
  end;
  VTV.FullExpand;
  VTV.EndUpdate;
end;

end.

So, Button1 uses my curent style of code, to build nodes. It does work.

Button2 is trying to call AddNode procedure to create AddNode as re-usable code. It compiles, but it fails on line #: 101: EAccessViolation:

I think there is a problem with the how I use, assign pointer and values... I didn't get past this line, so I don't know if rest of the code works.

Any suggestions how to fix this, how to make the code re-usable?

EDIT

If I remove vData parameter works good: This is now reduced code:

VTV.BeginUpdate;
  VTV.Clear;
  vLevelPrev:=0
  for i := Low(vArray) to High(vArray) do
  begin
    if i > 0 then vLevelPrev:=vArray[i-1].Level;

    Node:=AddNode(VTV,i,vArray[i].Level,vLevelPrev,Node);

    // Create link to your data record into VST node
    Data := VTV.GetNodeData(Node);
    Data.IndexInMyData := i;
    vArray[Data.IndexInMyData].NodePointer := Node;
  end;
  VTV.FullExpand;
  VTV.EndUpdate;

And AddNode:

function AddNode(vTV: TvirtualStringTree; vI, vLevel, vLevelPrev:integer; vNode:PVirtualNode):PVirtualNode;
var j:integer;
begin
   if vI = 0 then
    begin
      Result := vTV.AddChild(nil);
    end
    else
    begin
      if vLevel = 0 then  Result := vTV.AddChild(nil)
      else if vLevel > vLevelPrev then  Result := vTV.AddChild(vNode)
      else if vLevel < vLevelPrev then
      begin
        Result := vNode.Parent;
        for j := 1 to (vLevelPrev - vLevel) do
          Result := Result.Parent;
        Result := vTV.AddChild(Result);
      end
      else
      begin
        Result := vNode.Parent;
        Result := vTV.AddChild(Result);
      end;

    end;
end;

Anyway to reduce code by processing Data in AddNode?

SOLUTION:

I put Data as local pointer into AddNode:

function AddNode(vTV: TvirtualStringTree; vI, vLevel, vLevelPrev:integer; vNode:PVirtualNode):PVirtualNode;
var j:integer;
    Data: ^rTreeData;
begin
   if vI = 0 then
      Result := vTV.AddChild(nil)
    else
    begin
      if vLevel = 0 then  Result := vTV.AddChild(nil)
      else if vLevel > vLevelPrev then  Result := vTV.AddChild(vNode)
      else if vLevel < vLevelPrev then
      begin
        Result := vNode.Parent;
        for j := 1 to (vLevelPrev - vLevel) do
          Result := Result.Parent;
        Result := vTV.AddChild(Result);
      end
      else
      begin
        Result := vNode.Parent;
        Result := vTV.AddChild(Result);
      end;
    end;
    Data := VTV.GetNodeData(Result);
    Data.IndexInMyData := vI;
end;

And now I have the final reduced code that uses AddNode:

vLevelPrev := 0;
  for i := Low(vArray) to High(vArray) do
  begin
    if i > 0 then
      vLevelPrev := vArray[i - 1].Level;
    Node := AddNode(VTV, i, vArray[i].Level, vLevelPrev, Node);
    vArray[i].NodePointer := Node;
  end;

My question has now -1, I assume someone down-voted it? Why? I'm trying really hard to produce MCVE when I ask here. What can I improve? — Mike Torrettinni, Nov 22 '15 at 13:07
Find the parts that vary from one instance of the code to the others, and parameterize on that. Just like any other similar refactoring. Do you understand that concept? If so, what is the question? Have you tried to refactor and failed? — David Heffernan, Nov 22 '15 at 14:02
@DavidHeffernan I put working reduced code in Edit. Is this progress? — Mike Torrettinni, Nov 22 '15 at 14:21
This isn't a code review site. I don't know what you are looking for here. — David Heffernan, Nov 22 '15 at 14:45
Trying to reduce the repeated code, so it can be re-used whenever I use the same code - around 150 times in my app, for different arrays, different Virtual Treeviews. Trying to put Data variable and update it within AddNode, but can't make it work. To put this part of code `Data := VTV.GetNodeData(Node); Data.IndexInMyData := i;` into AddNode. — Mike Torrettinni, Nov 22 '15 at 15:00
So extract the method then. What do you what from us? You want us to write the code? — David Heffernan, Nov 22 '15 at 15:08
No, of course not. In my original example I show how I tried to put Data as parameter, but can't make it work. I can rewrite it, but it's right there. Should I put in Edit#2? — Mike Torrettinni, Nov 22 '15 at 15:14
OK, thank you for your time. I'll try better next time. I think I'm getting better at this with every question, but still room for improvement, i guess. — Mike Torrettinni, Nov 22 '15 at 15:21

score 4 · Accepted Answer · answered Nov 22 '15 at 13:41

4

At the call to AddNode in Button2Click the pointer variable Data is still uninitialized and points to some arbitrary memory, which will be written to inside AddNode leading to that access violation.

I am still not sure if I understand, why you need that vData parameter at all. Make vData a local pointer to a rTreeData record as is Data in Button1Click and remove that parameter completely. In Button2Click use I to index into vArray.

answered Nov 22 '15 at 13:41

Uwe Raabe

45,288
3
82
130

I'm just trying to put as much code in AddNode as possible. This is outside AddNode: `Data := VTV.GetNodeData(Node); Data.IndexInMyData := i; vArray[Data.IndexInMyData].NodePointer := Node;` - it would be great if I can put first 2 lines into AddNode. – Mike Torrettinni Nov 22 '15 at 13:48
I dont' know if I can remove Data... I use Data as link back to my vArray - let say OnClick for VTV I use: `Data := VTV.GetNodeData(VTV.FocusedNode); vArray[Data.IndexInMyData]...` – Mike Torrettinni Nov 22 '15 at 14:02
Aha, just got it, thank you! I just did that and it works great! – Mike Torrettinni Nov 22 '15 at 15:38

How to make this code re-usable

1 Answers1