Kiedy muszę stworzyć Destructor

Witam

Szukałem, ale nie mogłem znaleźć jasnej odpowiedzi.
Kiedy powinienem stworzyć destruktor dla klasy?

Przypuśćmy, że mamy klasę:

type
  TUser = class(TComponent)
  private
    FDataBase: TIBDatabase;
    FGrupa: TField;
  public
    property DataBase: TIBDatabase read FDataBase write FDataBase;
    property Grupa: TField read FGrupa write FGrupa;
  end;

Teraz pytanie czy powinienem pisać destruktor, który zwolni wszystkie dodane przeze mnie pola do klasy?
W stylu

destructor Destroy;
begin
	FDataBase.Free;
	FGrupa.Free;
	inherited;
end;

Jeśli nie muszę go pisać, to czy w sytuacji, gdy TUser dziedziczyłby po klasie TObject, zamiast z TComponent, to już byłaby potrzeba tworzenia destruktora?

Teraz pytanie czy powinienem pisać destruktor, który zwolni wszystkie dodane przeze mnie pola do klasy?

Tak, musisz napisać, w innym wypadku miałbyś wyciek pamięci; ogólnie zwalniane są tylko typy proste oraz ewentualnie* refcountowane (stringi oraz (dynamiczne) tablice), cała reszta pozostaje nietknięta.
* gdy refcount = 0.

Edit: no, chyba, że mowa o Delphi dla .NET lub po prostu kompilujesz program dla platformy z jakimś garbage-collectorem; wtedy zwalnianie pamięci nie powinno Cię w ogóle obchodzić.

Zasada jest taka, że skoro coś tworzysz - musisz to zwolnić (co innego, jesli jest GC), jest to bardzo dobra praktyka; Jeśli masz klasę z polami, które są tworzone w konstruktorze, to odpowiednio niszczysz te obiekty w destruktorze; Jeśli przydzielasz pamięć dynamicznie bazując na wskaźnikach - tak samo w destruktorze zwalniasz pamięć, dodatkowo wywołując destruktor bazowy klasy; Przykład:

type
  TFoo = class(TObject)
  private
    FList: TStrings;
  public
    constructor Create();
    destructor Destroy(); override;
  end;

{..}

constructor TFoo.Create();
begin
  inherited Create();
  FList := TStringList.Create();   // utworzenie
end;

destructor TFoo.Destroy();
begin
  FList.Free();   // zwolnienie
  inherited Destroy();
end;

chyba, że tak jak wspomniał @Patryk27 chcesz mieć wyciek pamięci.

EDIT: Tak nawiasem - w przedstawionej przez Ciebie klasie nie ma także konstruktora, więc FDataBase i FGrupa w ogóle nie zostaną utworzone;

Przy okazji - nie mieszaj angielskich i polskich identyfikatorów.

To mam kolejne pytanie.

type
	TLoginForm = class(TForm)
	private
		FUser: TUser;
	public	
		property User: TUser read FUser write FUser;	

W programie robię coś w stylu:

...
	form := TLoginForm.Create(Owner);
	form.ShowModal;
	form.Free; // Tutaj dostaje wyjątek: 'Invalid pointer operation'
...

Na Free formy dostaje Invalid pointer exception mimo, że Destructor TUser poprawiłem o sprawdzenie, czy pola są różne od nil przed ich Free.

Zbyt mało danych, nie można nic powiedzieć. Wrzuć trochę więcej kodu.

• debugger maybe?

plum napisał(a)

Na Free formy dostaje Invalid pointer exception mimo, że Destructor TUser poprawiłem o sprawdzenie, czy pola są różne od nil przed ich Free.

A czy jakakolwiek metoda tej klasy może te obiekty zwolnić wcześniej, niż w destruktorze? Bo jeśli tak, to zwalniaj je przez FreeAndNil i w destruktorze sprawdzaj czy są utworzone przez Assigned;

Jeśli zaś te obiekty nie mogą być wcześniej niż w destruktorze zwolnione - sprawdzanie czy są równe nil nie ma sensu, bo zawsze nie będą;

Być może w ogóle ich nie utworzyłeś w pamięci ani nie znilowałeś i stąd ten problem; Tak jak napisałem wcześniej - nie masz zaimplementowanego konstruktora, w którym ww. obiekty były by tworzone.

EDIT: Wypadałoby użyć debugera - wtedy na pewno znajdziesz błąd.

Utworzyłem taki trochę zagmatwany kod, ale tutaj jest właśnie dostaje błąd w destruktorze

uUser.pas

type
  TUser = class(TComponent)
  private
    FDataBase: TIBDatabase;
    FDostep: TField;
    FGrupa: TField;
    FKto: TField;
  public
    function Loguj: Boolean;
	
    destructor Destroy; override;

    property DataBase: TIBDatabase read FDataBase write FDataBase;
    property Dostep: TField read FDostep write FDostep;
    property Grupa: TField read FGrupa write FGrupa;
    property Kto: TField read FKto write FKto;
  end;

implementation

uses uLoginForm;

destructor TUser.Destroy;
begin
  if Self.DataBase <> nil then Self.FDataBase.Free;
  inherited;
end;

function TUser.Loguj: Boolean;
var
  LoginForm: TLoginForm;
begin
  Result := False;
  LoginForm := TLoginForm.Create(nil);
  if LoginForm.ShowModal = mrOk then
  begin
    Self.DataBase := TIBDatabase.Create(Self.Owner);
    Self.DataBase.DatabaseName := LoginForm.User.DataBase.DatabaseName;
    Self.DataBase.Params := LoginForm.User.DataBase.Params;
    Self.DataBase.LoginPrompt := False;
    Self.DataBase.Connected := True;

    Self.Dostep := LoginForm.User.Dostep;
    Self.Grupa := LoginForm.User.Grupa;
    Self.Kto := LoginForm.User.Kto;

    Result := True;
  end;
  LoginForm.Free;
end;

uLoginForm.pas

type
  TLoginForm = class(TForm)
    eHaslo: TEdit;
    btnLoguj: TButton;
    procedure btnLogujClick(Sender: TObject);
    procedure FormCreate(Sender: TObject);
  private
    liczProba: Integer;
    FUser: TUser;
  public
    property User: TUser read FUser write FUser;
  end;

var
  LoginForm: TLoginForm;

const logowanieCaption = 'Logowanie [Próba: %d]';

implementation

uses IBQuery;

{$R *.dfm}

procedure TLoginForm.btnLogujClick(Sender: TObject);
var
  query: TBQuery;
begin
  query := TIBQuery.Create(nil);
  query.SQL.Text := 'select * from UZYTKOWNICY where HASLO = '''+ UpperCase(eHaslo.Text) +'''';
  query.Open;

  if query.RecordCount = 1 then
  begin
    Self.User.Dostep := query.FieldByName('DOSTEP');
    Self.User.Grupa :=  query.FieldByName('GRUPA');
    Self.User.Kto := query.FieldByName('KTO');
    ModalResult := mrOk;
  end
  else
  begin
    Self.liczProba := Self.liczProba + 1;
    if Self.liczProba > 2 then
    begin
      ShowMessage('Próba włamania do systemu');
      ModalResult := mrAbort;
    end
    else
    begin
      Self.Caption := Format(logowanieCaption, [Self.liczProba]);
    end;
  end;
end;

procedure TLoginForm.FormCreate(Sender: TObject);
var
  iniFile: TIniFile;
  connectionString: string;
begin
  Self.liczProba := 0;

  try
    iniFile := TIniFile.Create(GetCurrentDir + '\SETTINGS.INI');
    connectionString := iniFile.ReadString('Baza danych', 'Serwer', '');
    iniFile.Free;

    Self.User := TUser.Create(Self);

    Self.User.DataBase := TIBDatabase.Create(Self);
    Self.User.DataBase.LoginPrompt := False;
    Self.User.DataBase.DatabaseName := connectionString;
    Self.User.DataBase.Params.Add('user_name=sysdba');
    Self.User.DataBase.Params.Add('password=masterkey');
    Self.User.DataBase.Connected := true;
  except
    on e: Exception do
    begin
      ShowMessage(e.Message);
      Application.Terminate;
    end;
  end;
end;

Wywołanie

var user: TUser;
begin
	user := TUser.Create(Self);
	if user.Loguj then
		ShowMessage('Zalogowany');
end;

Wiem, że zamotałem trochę, ale chciałem sobie wszystko zaszyć w klasie TUser.

Kod jest tak niedbale napisany, że praktycznie wszędzie grozi wystąpienie wycieków pamięci; Jeśli coś tworzysz dynamicznie to to zwalniaj i wspomóż się blokiem Try Finally;

Poza tym tak jak przypuszczałem - nie masz konstruktora; Więc albo twórz obiekt FDataBase w konstruktorze i zwalniaj go w destruktorze, albo twórz go dynamicznie i w tej samej metodzie go zwalniaj;

Btw - niepotrzebnie wszędzie wpisujesz Self; Jak już koniecznie chcesz mieć pewność, że odwołujesz się do jego zawartości - wpakuj wszystko do instrukcji wiążącej With.

Debugger, debugger, debugger... "przeleć" każdą linijkę po kolei sprawdzając wartości zmiennych, sprawdź czy są poprawnie inicjowane etc.
Btw, to self. jest zbędne w większości przypadków.

ReportMemoryLeaksOnShutdown := True;

może pomoże namierzyć winowajce.

dodanie znacznika <code class="delphi"> - fp

Problem 1:

Self.DataBase := TIBDatabase.Create(Self.Owner);

Zamień na:

Self.DataBase := TIBDatabase.Create(Self);

lub

Self.DataBase := TIBDatabase.Create(nil);

Problem 2:

Jeśli:

    Self.User.Dostep := query.FieldByName('DOSTEP');
    Self.User.Grupa :=  query.FieldByName('GRUPA');
    Self.User.Kto := query.FieldByName('KTO');

to obsłuż Notification / opRemove i czyść referencje do pól gdy są usuwane z pamięci.

Problem 3:

Zamiast:

if Self.DataBase <> nil then Self.FDataBase.Free;

możesz napisać:

FreeAndNil(FDataBase);

a nawet

// nic - AFAIR FDatabase zostanie zwolnione automatem, jeśli w konstruktorze podasz jako ownera Self.

Problem 4:
"Zniszcz" globalną nazwę obiektu formy - wersja pierwotna prowadzi do tego że masz dwie zmienne o tej samej nazwie, co może utrudniać znalezienie błędu:

Zamiast:

var
  LoginForm: TLoginForm;

napisz:

var
  gLoginForm: TLoginForm; // dla zmiennej globalnej

Problem 5:
W kodzie klasy TUser tworzysz formę, która tworzy obiekt... TUser.

procedure TLoginForm.FormCreate(Sender: TObject);
begin
//...
Self.User := TUser.Create(Self);
//...
end;

Trzeba to rozplątać, chyba że dopuszczasz do sytuacji że są dwa obiekty TUser - tylko czy to ma sens?

To ja może dodam, że w TLoginForm tworzysz drugiego TUsera, potem przekopiowujesz z niego dane, ale go nie zwalniasz z pamięci.
Najlepiej, żebyś to co jest w TLoginForm.FormCreate przeniósł do konstruktora TUsera (oczywiście bez linijki z TUser.Create). Wtedy w TUser.Loguj albo kopiujesz tylko: Dostep, Grupa i Kto. Albo przekazujesz wskaźnik TUsera do TLoginForm, i nic nie kopiujesz tylko od razu się wypełnia.
Mniej więcej tak:

function TUser.Loguj: Boolean;
var
  LoginForm: TLoginForm;
begin
  LoginForm := TLoginForm.Create(nil);
  try
    LoginForm.User := Self;
    Result :=  LoginForm.ShowModal = mrOk;
  finally
    LoginForm.Free;
  end;
end;

Chociaż wywoływanie formy z klasy TUser wydaje mi się nieeleganckie. Ja bym zrobił raczej funkcję: Zaloguj(User: TUser): boolean;

Lepiej przerób na to:

class function TLoginForm.Execute(AUser:TUser):Boolean;
begin
  with Create(Application) do
  begin
    try
      User:=AUser; // to nie koniecznie ale kompatybilne z twoim
      Result:=(ShowModal=mrOk);
      //if Result then AUser.Property1:=Property1; // to lepsza alternatywa
    finally
      Free;
    end;
  end;
end;