zusammenfalten · markieren

Delphi-Quellcode:

{...}
  FPaperTrays : TStrings;
{...}

function TPrinterCustom.GetPaperTrays: TStrings;
var
  pDevMode : PDeviceMode;
  bin : array[0..255,0..23] of Char;
  i : DWORD;
  Res : DWORD;
  PPrinter : PPrinterInfo2;
begin
  if (FPaperTrays = nil) then
  begin
    try
      FPaperTrays := TStringList.Create;
    except
      FPaperTrays.Free;
      FPaperTrays := nil;
    raise;
    end;
  end;
  FPaperTrays.Clear;
  if FAllInfos.Count <> 0 then
  begin
    PPrinter := FAllInfos.Items[FPrinterIndex];
    pDevMode := nil;
    Res := DeviceCapabilities(PPrinter^.pPrinterName,
                              PPrinter^.pPortName,
                              DC_BINNAMES,
                              PChar(@(bin[0][0])),
                              pDevMode);
    for i := 0 to Res-1 do
      FPaperTrays.Append(bin[i]);
  end;
  Result := FPaperTrays;
end;

Ich habe beim zweiten oder dritten Mal aufrufen das Problem, das ich einen AccessViolation-Fehlermeldung bekomme in der FOR-Schleife. Desweiteren wird das Ergebnis auch nicht wie gewollt mit den neu ermittelten Werten überschrieben.

Was mache ich falsch und was könnte man besser machen ? Das Ergebnis soll eine Liste mit allen Papierschächten des Druckers sein.

Eine Sache machst du schon mal falsch (weiß aber nicht ob's daran liegt)

Und zwar: wenn im Konstruktor ne Exception geworfen wird, dann darf der Destruktor nicht aufgerufen werden. In diesem Fall sorgt Delphi schon dafür dass das nichts überigbleibt, ein .Free könnte also in einer AV münden.

Wird eventuell irgendwo die Liste, die du zurückgibts mit .Free zerstört? Das könnte sehr gut die Fehlerursache sein. Ich glaube nämlich der Aufruf von TStringlist.Create dürfte fast nie fehlschlagen.
Falls FPaperTrays ein privates feld deiner Klasse ist, würde ich das auch nicht "herausgeben" - besser wäre hier ein Parameter const x:TStrings in den das hineingeschrieben wird. Dann brauchst du dich auch nicht mehr um den Konstruktor kümmern.

Der Code:

markieren

Delphi-Quellcode:

if (FPaperTrays = nil) then
  begin
    try
      FPaperTrays := TStringList.Create;
    except
      FPaperTrays.Free;
      FPaperTrays := nil;
    raise;
    end;
  end;

ist Blödsinn. Wenn nichts erzeugt werden kann, kann auch nichts freigegeben werden.

Das Reicht:

markieren

Delphi-Quellcode:

if (FPaperTrays = nil) then
  FPaperTrays := TStringList.Create;
else
  ...;
  ...;

Wie erzeugst du die StringListe und wie gibst du sie wieder frei? das sind nicht ganz irrelevante Codeteile.

Ich würde aus der Funktion eine Prozedur machen, die ein TStrings-Objekt als Parameter annimmt. Somit ist die aufrufende Routine für die Instanzierung verantwortlich.

Zitat von Luckie:

Wie erzeugst du die StringListe und wie gibst du sie wieder frei? das sind nicht ganz irrelevante Codeteile.

Diese Funktion ist eine protected-Funktion in einer Klasse. Ich benutze die TStringListe nur zur Übergabe an das Result. Für mehr nicht.

Zitat von DeddyH:

Ich würde aus der Funktion eine Prozedur machen, die ein TStrings-Objekt als Parameter annimmt. Somit ist die aufrufende Routine für die Instanzierung verantwortlich.

Das heißt, Du würdest bei Listen in einer Klasse, das ganze besser in eine Procedure in den Public-Teil packen ?

Wenn das Methoden einer Klasse sind, warum kennen sie dann nicht die StringListe? Da braucht man doch gar nichts zu übergeben und andauert wieder neu zu erstellen, nur bei Bedarf leeren.

Achso, die ist im protected teil, ja dann aist das was anderes

Ich tippe mal, du hast noch irgendwo ein FPaperTrays.Free; stehen - das wird aufgerufen, und das Objekt ferigegeben. Sobald dann die Methode nochmal aufgerufen wird, knallts.
Denk' dran: ein ".Free" setzt den Zeiger nicht zurück auf nil, d.h. deine Abfrage ist nur ganz am Anfang beim ersten Mal von Bedeutung. Und dann könnte man das erstellen der Stringliste eigentlich auch in den Konstruktor packen und nur noch leeren und wiederbefüllen.

Moin Rolf,

die Umsetzung von Detlefs Idee würde, z.B., so aussehen:

zusammenfalten · markieren

Delphi-Quellcode:

procedure TPrinterCustom.GetPaperTrays(const APaperTrays : TStrings);
var
  pDevMode : PDeviceMode;
  bin : array[0..255,0..23] of Char;
  i : DWORD;
  Res : DWORD;
  PPrinter : PPrinterInfo2;
begin
  APaperTrays.Clear;
  if FAllInfos.Count <> 0 then
  begin
    PPrinter := FAllInfos.Items[FPrinterIndex];
    pDevMode := nil;
    Res := DeviceCapabilities(PPrinter^.pPrinterName,
                              PPrinter^.pPortName,
                              DC_BINNAMES,
                              PChar(@(bin[0][0])),
                              pDevMode);
    for i := 0 to Res-1 do
      APaperTrays.Append(bin[i]);
  end;
end;

Vorteil:
Man erhält eine klarere Struktur für das Erzeugen und Freigeben der Stringliste.

Ganz genau. Ich würde nur noch in der ersten Zeile auf Assigned prüfen.

markieren

Delphi-Quellcode:

if not Assigned(APaperTrays) then
  raise Exception.Create('Was soll ich denn hier füllen?');

Danke Christian, Danke Detlef. Ich glaube ich werde in der Klasse alle TStrings-Rückgaben so umschreiben wie Christian es gemacht hat. Dann habe ich doch eine saubere Struktur drin. Oder gibt es dagegen etwas einzuwenden ?