2017-08-25 16 views
4

this質問に加えて、私はいくつかのテストとdocwikiの研究を行いました。Delphi関数戻りクラスオブジェクト

function testResultObject: TClassA; 
begin 
Result := TClassA.Create; 
Result.DoSomething; 
end; 

そしてどこかに、私はこのように上記のコードを呼び出すことができます:私の結論は、この種のコードは、メモリリークせずに動作するはずということであるレミーはそれがより良いの答えで提案されているように

var k: TClassA; 
begin 

k := testResultObject; 
try 
    //code code code 
finally 
    k.Free; 
end; 

end; 

をこのようなことを避け、testResultObject(x: TClassA): booleanのようなものを使用してください。この場合、true/falseを返すと、すべてがうまくいって、すでに作成されたオブジェクトを渡していることがわかります。このコードでは

ルック:

function testResultObject: TClassA; 
begin 

Result := TClassA.Create; 

try 
    Result.DoSomething; 
except 
    Result.Free; 
end; 

end; 

機能の上記の最初のバージョンの問題はDoSomethingは例外を発生させる可能性があり、もしそうなら、私はメモリリークだろうということです。 try-exceptで2番目の実装を解決することはできますか?確かに後で結果が割り当てられているかどうかを確認する必要があります。

私は(上記のとおり)testResultObject(x: TClassA): booleanが良いと同意します。私が書いたように、クラスを返す関数の方法を修正できるかどうかは疑問だった。

+2

個人的に私はいつもより良いコードを読みやすくする第2のアプローチ(true/false return)を使用してきました。あなたの解答は良いですが、例外ブロックにResult:= nilも追加してください。 –

+0

あなたは以前の答えを誤解しています。オブジェクトをパラメータとして渡すことは、そのオブジェクトを変更しているとき(この例では、リストに項目を追加するとき)に有効です。これはまったく異なるシナリオです。 –

答えて

9

コードに深刻な問題があります。エラーが発生した場合、例外をスワップし、無効なオブジェクト参照を返します。

これは簡単に修正できます。正規の方法は次のとおりです。

function testResultObject: TClassA; 
begin 
    Result := TClassA.Create;  
    try 
    Result.DoSomething; 
    except 
    Result.Free; 
    raise; 
    end; 
end; 

この関数はいずれも成功し、新しいオブジェクトを返します。または、失敗し、それ自体の後でクリーンアップし、例外を発生させます。

つまり、この関数はコンストラクタのように見え、動作します。

obj := testResultObject; 
try 
    // do things with obj 
finally 
    obj.Free; 
end; 
+2

これは最高の答えだと思います。私は標準的なやり方で物事をやりたいのですが、ここでは通常のtry-finallyを使うことができます。また、関数は適切にクリーンアップを行い、コードは読みやすくなります。 –

4

これの唯一の問題点:

function testResultObject: TClassA; 
begin 
    Result := TClassA.Create;  
    try 
    Result.DoSomething; 
    except 
    Result.Free; 
    end; 
end; 

は、関数が成功したかどうかを知る方法はありませんということです。オブジェクトを解放しても参照は変更されません。変数は、オブジェクトが存在していた(今)無効なメモリ位置を依然として指しています。消費者が参照が有効であるかどうかをテストできるようにするには、参照をnilに明示的に設定する必要があります。あなたは(nilに対する消費者のテストを有している)、このパターンを使用したい場合、あなたは何をする必要があります:

try 
    Result.DoSomething; 
except 
    FreeAndNil(Result); 
end; 

この方法で呼び出し側は、あなたが意図したとおり(Assignedまたはその他を使用して)nilのための結果をテストすることができます。しかし、これはまだあなたが例外を飲み込んでいるので、非常にきれいなアプローチではありません。別の解決法は、単純に新しいコンストラクタを導入するか、既存のコンストラクタを変更することです。例

TFoo = class 
    public 
    constructor Create(ADoSomething : boolean = false); 
    procedure DoSomething; 
end; 

constructor TClassA.Create(ADoSomething: Boolean = False); 
begin 
    inherited Create; 
    if ADoSomething then DoSomething; 
end; 

procedure TClassA.DoSomething; 
begin 
    // 
end; 

についてはあなたが例外処理のすべてを取り除くと同じように、これを呼び出して取得することができますこの方法:あなたが今、コンストラクタに意志自然に例外をDoSomething実行をプッシュしたので

function testResultObject: TClassA; 
begin 
    Result := TClassA.Create(true);  
end; 

自動的にデストラクタを呼び出すと、メモリ管理の問題は解消されます。他の答えにも良い解決策があります。

+0

なぜ私はあなたがそのような例外を飲み込むことを提案するのか理解できません。 –

+0

@DavidHeffernan私はそれを示唆しません。だから私はより賢明な選択肢を追加しました。 OPはこのパターンを示唆し、彼らはそれの欠点を十分に認識していたことを示していたので、私はそのポイントを告げる必要はないと感じました。 –

+0

私はラファエレがこれをまったく感謝しているとは思わない。漏れに集中している質問には言及していない。あなたの答えの前半は、私がそれを読んでいる間、例外の嚥下を祝福します。それは間違っている。そしてリレイズで簡単に修正できます。 –

5

2番目の方法は機能しますが、2つの重大な問題があります。

  • (Jが指摘したように)すべての例外を飲み込むことで、何かが間違っていたという事実を隠すことになります。
  • 呼び出し元が、呼び出し元が破棄するオブジェクトを作成したことを示す呼び出しはありません。これは関数をよりエラーを起こしやすいようにする。メモリリークを起こしやすくなります。

私はあなたの第二のアプローチで、次の改善をお勧めします:上記の最も重要な利点は、関数を作成

  {Name has a clue that caller should take ownership of a new object returned} 
function CreateObjectA: TClassA; 
begin 
    {Once object is successfully created, internal resource protection is required: 
     - if no error, it is callers responsibility to destroy the returned object 
     - if error, caller must assume creation *failed* so must destroy object here 
    Also, by assigning Result of successful Create before *try*: 
     The object (reference) is returned 
      **if-and-only-if** 
     This function returns 'normally' (i.e. no exception state)} 
    Result := TClassA.Create;  
    try 
    Result.DoSomething; {that could fail} 
    except 
    {Cleanup only if something goes wrong: 
     caller should not be responsible for errors *within* this method} 
    Result.Free; 
    {Re-raise the exception to notify caller: 
     exception state means caller does not "receive" Result... 
     code jumps to next finally or except block} 
    raise; 
    end; 
end; 

をということである:限りすべての呼び出し元/クライアントコードなどが懸念され、それ通常のTObject.Createのように正しく動作します。
正しいの使用パターンはまったく同じです。

私はJのFreeAndNil提案に熱心ではないことに注意してください。なぜなら、呼び出しコードが結果が割り当てられているかどうかをチェックしないと、AVになる可能性が高いからです。

var k: TClassA; 
begin 
    k := testResultObject; {assuming nil result on failed create, next/similar is *required*} 
    if Assigned(k) then {Note how this differs from normal try finally pattern} 
    try 
    //code using k 
    finally 
    k.Free; 
    end; 
end; 

NB:と正しく結果をチェックしないコードは少し厄介になりますそれはあなたがあなたの呼び出し側が単純にメモリ管理を無視することはできませんことに注意することが重要です。それは私を次のセクションに連れて行きます。


上記のすべてはさておき、あなたのtestResultObjectは、作成し、必要に応じてその寿命を管理するために、発信者が必要な入力オブジェクトを取る場合は不注意な間違いをはるかに少ない可能性があります。 なぜあなたはそのアプローチにそんなに抵抗しているのか分かりません。 異なるメモリモデルを使用せずに、以下より単純にすることはできません。

var k: TClassA; 
begin 
    k := TClassA.Create; 
    try 
    testResultObject(k); {Where this is simply implemented as k.DoSomething;} 
    //more code using k 
    finally 
    k.Free; 
    end; 
end; 
+0

私が言ったように、私はtestResultObject(x:TClassA)のようなものを好む:TClassAを返すのではなくブール値。私の解決策が正しいかどうかを理解するのはちょっと不思議でした;)あなたの最後の実装が最高だと私は同意します! –

+1

@Raffaeleなぜブール値が好きですか?ユーザーにテストを強制し、例外を飲み込む。これを修正し、呼び出しコードを標準パターンに従うようにする簡単な方法があります。 –

+0

@Craig最後の提案は、オブジェクトをインスタンス化する前に関数が例外を発生させるシナリオを解決しません。それがあなたの最初のアプローチが勝つ場所です。 –