How should I implement GetLastNode for TTreeNodes?

When I need to find the first node in a TTreeView, I call TTreeNodes.GetFirstNode. However, I sometimes need to locate the last node in a tree and there is no corresponding TTreeNodes.GetLastNode function.

I don't want to use Items[Count-1] since that results in the entire tree being walked with Result := Result.GetNext. Naturally this only matters if the tree views have a lot of nodes. I fully appreciate the virtues of virtual container controls but I am not going to switch to Virtual TreeView just yet.

So far I have come up with the following:

function TTreeNodes.GetLastNode: TTreeNode;
var
  Node: TTreeNode;
begin
  Result := GetFirstNode;
  if not Assigned(Result) then begin
    exit;
  end;
  while True do begin
    Node := Result.GetNextSibling;
    if not Assigned(Node) then begin
      Node := Result.GetFirstChild;
      if not Assigned(Node) then begin
        exit;
      end;
    end;
    Result := Node;
  end;
end;

Can anyone:

1. Find a flaw in my logic?
2. Suggest improvements?

Edit 1

I'm reluctant to keep my own cache of the nodes. I have been doing just that until recently but have discovered some开发者_运维问答 hard to track very intermittent AVs which I believe must be due to my cache getting out of synch. Clearly one solution would be to get my cache synchronisation code to work correctly but I have an aversion to caches because of the hard to track bugs that arise when you get it wrong.

Although I am not a non-Exit purist, I think that when it is doable without Exit while keeping readability intact, one might prefer that option.

So here is exactly the same code, for I don't think you can get any other way (faster) to the end node, but without Exit and slightly more compact:

function TTreeNodes.GetLastNode: TTreeNode;
var
  Node: TTreeNode;
begin
  Node := GetFirstNode;
  Result := Node;
  if Result <> nil then
    repeat
      Result := Node;
      if Node <> nil then
        Node := Result.GetNextSibling;
      if Node = nil then
        Node := Result.GetFirstChild;
    until Node = nil;
end;

The method I've used before is the maintain a TList using List.Add on the OnAddition event and List.Remove on the OnDeletion event (OnRemove?). You've access to List.Count-1 (or whatever you need) pretty much instantly then.

Post edit - I have to say that although this worked fine, I then grew up and moved to Virtual Tree View :-)

If I was to implement it, this would probably be my first draft.

function TTreeNodes.GetLastNode: TTreeNode;
var
  Node: TTreeNode;
  function GetLastSibling(aNode : TTreeNode) : TTreeNode;
  begin
    if not Assigned(aNode) then
      EXIT(nil);
    repeat
      Result := aNode;
      aNode := Result.GetNextSibling;
    until not Assigned(aNode) ;
  end;
begin
  Node := GetFirstNode;
  if not Assigned(Node) then begin
    exit;
  end;

  repeat
    Result := GetLastSibling(Node);
    Node := Result.GetFirstChild;
  until not Assigned(Node);
end;

I find this slightly more readable. It might be slightly slower though.

I'm unsure about whether or not this approach would be faster than items[Count-1], in some cases, it could be slower, as the TTreeNodes actually caches the last node accessed through the items property.