Jump to content
AlanScottAgain

Interfaces - Time to face my ignorance.

Recommended Posts

Posted (edited)

I know I should have done it sooner - I've been putting it off too long.

 

Thanks to PB for giving me a push to do this, in another tread.

So I've  stripped down my TDrawingObject and TDrawingObjectList Classes to show what I have and what I'm trying to acheive :

Type
  TDrawingObjectType = (doChild, doBackground);

 

 TDrawingObjectsList = Class; //Forward Declaration

 

  TDrawingObject = Class
  Private
    FObjectType: TDrawingObjectType;
    FDrawingObjectsList: TDrawingObjectsList;
  Public
    Constructor Create(AObjectType: TDrawingObjectType);
    Destructor Destroy;
    Property ObjectType: TDrawingObjectType Read FObjectType Write FObjectType;
    Property DrawingObjectsList: TDrawingObjectsList Read FDrawingObjectsList Write FDrawingObjectsList;
  End;

 

 

  TDrawingObjectsList = Class(TObjectList<TDrawingObject>)
  Private
    FOwnsObjects: Boolean;
  Protected
    Function GetItem(Index: Integer): TDrawingObject;
    Procedure SetItem(Index: Integer; AObject: TDrawingObject);
  Public
    Procedure Draw(ACanvas: IskCanvas);
    Property OwnsObjects: Boolean Read FOwnsObjects Write FOwnsObjects;
    Property Items[Index: Integer]: TDrawingObject Read GetItem Write SetItem; default;
    Procedure LoadDrawingObjects;
    
  End;

 

I'm trying to implement interfaces for these objects The code below looks pretty good at least I don't a lot of errors:

 

I have two main questions at the moment:


In the TDrawingListClass declaration  I used to have TDrawingObjectsList = Class(TObjectList<TDrawingObject>) to type cast the list. How do I achieve the same with the class/interface declaration?  TDrawingObjectsList = class(TInterfacedObject, IDrawingObjectList, TObjectList<TDrawingObject>) is not right.

In the class constructs I have kept the interface references eg: property DrawingObjectList: IDrawingObjectList read GetDrawingObjectList write SetDrawingObjectList;     Is this correct?

 

type
  TDrawingObjectType = (doChild, doBackground);

 

  IDrawingObjectList = interface; // forward declaration

 

  IDrawingObject = interface(IInterface)
    ['{A8701C68-51FC-4087-8C30-E3CA4C7C1D8E}']
    function GetDrawingObjectType: TDrawingObjectType;
    procedure SetDrawingObjectType(ADrawingObjectType: TDrawingObjectType);
    function GetDrawingObjectList: IDrawingObjectList;
    procedure SetDrawingObjectList(ADrawingObjectList: IDrawingObjectList);
    property ObjectType: TDrawingObjectType read GetDrawingObjectType write SetDrawingObjectType;
    property DrawingObjectList: IDrawingObjectList read GetDrawingObjectList write SetDrawingObjectList;
  end;

 

  TDrawingObject = class(TInterfacedObject, IDrawingObject)
    function GetDrawingObjectType: TDrawingObjectType;
    procedure SetDrawingObjectType(ADrawingObjectType: TDrawingObjectType);
    function GetDrawingObjectList: IDrawingObjectList;
    procedure SetDrawingObjectList(ADrawingObjectList: IDrawingObjectList);
    property ObjectType: TDrawingObjectType read GetDrawingObjectType write SetDrawingObjectType;
    property DrawingObjectList: IDrawingObjectList read GetDrawingObjectList write SetDrawingObjectList;

  end;

 

  IDrawingObjectList = interface(IInterface)
    ['{9D2C916D-19F0-49B2-93C6-B84592442057}']
    Private

    function GetOwnsObject: Boolean;
    procedure SetOwnsObject(AOwnsObject: Boolean);
    property OwnsObjects: Boolean read GetOwnsObject write SetOwnsObject;
    function GetItem(Index: Integer): IDrawingObject;
    procedure SetItem(Index: Integer; AObject: IDrawingObject);
    Public

    Constructor Create(AObjectType: TDrawingObjectType);
    Destructor Destroy;

    property Items[Index: Integer]: IDrawingObject read GetItem Write SetItem;

    procedure Draw(ACanvas: ISkCanvas);
    procedure LoadDrawingObjects;
  end;

 

  TDrawingObjectsList = class(TInterfacedObject, IDrawingObjectList)
  private
    FOwnsObjects: Boolean;
    function GetOwnsObject: Boolean;
    procedure SetOwnsObject(AOwnsObject: Boolean);
    property OwnsObjects: Boolean read GetOwnsObject write SetOwnsObject;
    function GetItem(Index: Integer): IDrawingObject;
    procedure SetItem(Index: Integer; AObject: IDrawingObject);
  public
    property Items[Index: Integer]: IDrawingObject read GetItem write SetItem;

    procedure Draw(ACanvas: ISkCanvas);
    procedure LoadDrawingObjects;
  end;

 

Thanks

Alan

Edited by AlanScottAgain

Share this post


Link to post

If you are having trouble managing objects memory, then using interfaces is a bad option as using them requires even more knowledge. Yes, they can manage memory for you, but there is so much fine print attached I wouldn't recommend it as a starting point.

 

Your drawing object list class also has some unnecessary code as it inherits from generic TObjectList that already automatically handles all that. I have simplified your code and added some basic workflow.

 

uses
  System.Generics.Collections;

type
  TDrawingObjectType = (doChild, doBackground);

  TDrawingObjectsList = class; //Forward Declaration

  TDrawingObject = class
  private
    FObjectType: TDrawingObjectType;
    FDrawingObjectsList: TDrawingObjectsList;
  public
    constructor Create(AObjectType: TDrawingObjectType);
    destructor Destroy; override;
    property ObjectType: TDrawingObjectType read FObjectType write FObjectType;
    property DrawingObjectsList: TDrawingObjectsList read FDrawingObjectsList;
  end;

  TDrawingObjectsList = class(TObjectList<TDrawingObject>)
  public
    procedure DrawDraw(ACanvas: IskCanvas);
    procedure LoadDrawingObjects;
  end;

  procedure BuildList(List: TDrawingObjectsList);

var
  Root: TDrawingObject;

implementation

constructor TDrawingObject.Create(AObjectType: TDrawingObjectType);
begin
  FDrawingObjectsList := TDrawingObjectsList.Create(True);
end;

destructor TDrawingObject.Destroy;
begin
  FDrawingObjectsList.Free;
  inherited;
end;

procedure BuildList(List: TDrawingObjectsList);
var
  Item: TDrawingObject;
begin
  List.Clear;

  // build list

  Item := TDrawingObject.Create(doChild);
  // set Item data
  // ...
  // Add Item to List
  List.Add(Item);

  Item := TDrawingObject.Create(doChild);
  // set Item data
  // ...
  // Add Item to List
  List.Add(Item);
end;


initialization

  Root := TDrawingObject.Create(doBackground);
  BuildList(Root.DrawingObjectsList);

finalization

  Root.Free;

end.

 

FDrawingObjectsList list will handle lifetime of any drawing object added to the list, so you don't have to deal with them. Please note that you cannot add same drawing object to multiple lists as it will be released twice, causing exception. Next, FDrawingObjectsList belongs to its drawing object instance and as such it should be constructed in constructor and destroyed in destructor of TDrawingObject. Nobody else should free that list nor assign anything to its reference there is no reason for it to be writeable property. Constructing/destroying part is the simplest and safest possible, there are other more complex coding patterns that involve lazy initialization, but they require more code and there is more room for bugs. Unless you need to preserve memory as much as possible, you don't need to deal with more complicated code. And if needed you can easily add that later on.

 

There is additional mistake in your destructor declaration. You are missing override directive. Without it destructor will never be called and you will have memory leaks.

 

I have created global Root object and populated it in initialization section. You don't have to make it a global, this is just to illustrate how to work with Root object and how to populate its data. BuildList takes TDrawingObjectsList as a parameter and populates existing list without constructing new one. if the list already has some items it will be cleared - you can omit clearing the list if you know you will populate it only once, or you can clear it in outside code before calling BuildList.

 

I am not going to comment on the interface part as it would be a really long write.

 

  • Like 1

Share this post


Link to post
36 minutes ago, AlanScottAgain said:

I'm trying to implement interfaces for these objects

Why? What problem are you trying to solve?

 

While I'm definitely a proponent of interfaces at a higher level, at low level they often just obfuscate the code.

 

Apart from that I think your solution has a number of "smells":

  • Why is the TDrawingObject.ObjectType writable? Does the ObjectType really need to change after construction?
  • Why is TDrawingObject.DrawingObjectsList writable? Who owns the list? (might have been covered before. TL;DR)
  • Instead of exposing the whole TObjectList<> class, expose only the relevant parts (Add, Remove, Enumerator). See:
  • Why is TDrawingObjectsList.OwnsObjects part of the API? Isn't the ownership an implementation detail?
  • Like 2

Share this post


Link to post

Thanks for all the feedback.

 

I think that interfaces at this stage is overkill.

 

As a result of the comments I have cleaned up the code and taken the advice on object lifetime and ownership.


 

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×