13

私は現在コードレビューを行っており、次のコードでジャンプしました。このコードには複数の問題があります。私に賛成してくれますか？もしそうなら、これが間違っていることを同僚にどのように説明できますか (頑固なタイプ...)?

  • 一般的な例外をキャッチする (Exception ex)
  • 別の catch ブロックを持つ代わりに「if (ex is something)」を使用する
  • SoapException、HttpException、および WebException を食べます。しかし、Web サービスに障害が発生した場合は、何もする必要はありません。

コード：

try
{
    // Call to a WebService
}
catch (Exception ex)
{
    if (ex is SoapException || ex is HttpException || ex is WebException)
    {
        // Log Error and eat it.
    }
    else
    {
        throw;
    }
}
4

8 に答える 8

17

マントラは次のとおりです。

  • 適切に処理できる場合にのみ、例外をキャッチする必要があります

したがって：

  • 一般的な例外をキャッチしないでください。

あなたの場合、はい、それらの例外をキャッチして、何か役立つことをする必要があります（おそらくそれらを食べるだけでなく、throwログに記録した後にできます）。

あなたのコーダーはthrow（ではないthrow ex）を使用していますが、これは良いです。

これは、複数の特定の例外をキャッチする方法です。

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    // Log Error and eat it
}
catch (HttpException ex)
{
    // Log Error and eat it
}
catch (WebException ex)
{
    // Log Error and eat it
}

これは、コードが行うこととほとんど同じです。あなたの開発者は、「エラーをログに記録してそれを食べる」ブロックの重複を避けるために、おそらくそのようにしました。

于 2009-01-08T22:47:21.083 に答える
7

私は現在コードレビューを行っていますが、次のコードでジャンプしました。このコードには複数の問題があります。私に賛成してくれますか？

完全ではありません。以下を参照してください。

  • 一般的な例外をキャッチします（例外例）

一般に、一般的な例外をキャッチすることは、それを処理できないという結論に達したときに（throw;を使用して）再スローする限り、実際には問題ありません。コードはそれを行うので、ここではすぐに問題はありません。

  • 別のキャッチブロックを使用する代わりに、「if（ex issomething）」を使用する

catchブロックの正味の効果は、SoapException、HttpExceptionなどのみが実際に処理され、他のすべての例外が呼び出しスタックに伝播されることです。機能的にはこれがコードの機能だと思うので、ここでも問題はありません。

ただし、美学と読みやすさのPOVから、「if（ex is SoapException || ..）」よりも複数のキャッチブロックを使用することをお勧めします。一般的な処理コードをメソッドにリファクタリングすると、複数のcatchブロックの入力がわずかに増え、ほとんどの開発者にとって読みやすくなります。また、最後のスローは見落とされがちです。

  • SoapException、HttpException、WebExceptionを食べます。しかし、Webサービスが失敗した場合、やることはあまりありません。

ここにコードの最大の問題が潜んでいる可能性がありますが、アプリケーションの性質について詳しく知らずにアドバイスを与えることは困難です。Webサービス呼び出しが後で依存することを実行している場合は、例外をログに記録して食べるのはおそらく間違っています。通常、例外を呼び出し元に伝播させます（通常、XyzWebServiceDownExceptionなどにラップした後）。Webサービス呼び出しを数回再試行した後でも（偽のネットワーク問題がある場合はより堅牢にするため）。

于 2009-01-09T13:13:55.367 に答える
6

同じ例外をキャッチして再スローする場合の問題は、.NET はスタック トレースをそのまま維持するために最善を尽くしますが、常に変更されてしまうため、例外が実際にどこから発生したかを追跡することがより困難になる可能性があることです (例: 例外行番号は、例外が最初に発生した行ではなく、再スロー ステートメントの行に表示される可能性があります)。キャッチ/再スロー、フィルタリング、キャッチしないことの違いについては、さらに多くの情報があります。

このような重複したロジックがある場合、本当に必要なのは例外フィルターです。そのため、関心のある例外の種類のみをキャッチできます。VB.NET にはこの機能がありますが、残念ながら C# にはありません。仮想的な構文は次のようになります。

try
{
    // Call to a WebService
}
catch (Exception ex) if (ex is SoapException || ex is HttpException || /* etc. */)
{
    // Log Error and eat it
}

ただし、これを行うことはできないため、代わりに私がよく行うのは、共通コードにラムダ式を使用することです ( delegateC# 2.0 では a を使用できます)。

Action<Exception> logAndEat = ex => 
    {  
        // Log Error and eat it
    };

try
{
    // Call to a WebService
}
catch (SoapException ex)
{
    logAndEat(ex);
}
catch (HttpException ex)
{
    logAndEat(ex);
}
catch (WebException ex)
{
    logAndEat(ex);
}
于 2009-01-08T23:40:26.797 に答える
2

これは、実際にすべての例外をキャッチすることなく、「すべての例外をキャッチする」シナリオを処理する唯一の方法である場合があります。

たとえば、低レベルのフレームワーク/ランタイムコードでは、例外がエスケープされないようにする必要がある場合があります。残念ながら、フレームワークコードが、スレッドによって実行されたコードによって発生した例外を知る方法もありません。

その場合、考えられるキャッチブロックは次のようになります。

try
{
   // User code called here
}
catch (Exception ex)
{
   if (ExceptionIsFatal(ex))
     throw;

   Log(ex);
}

ただし、ここには3つの重要なポイントがあります。

  1. これはすべての状況に適しているわけではありません。コードレビューでは、これが実際に必要であり、したがって許可されている場所について非常に慎重です。
  2. ExceptionIsFatal（）メソッドは、決して飲み込まれてはならない例外（ExecutionEngineException、OutOfMemoryException、ThreadAbortExceptionなど）を食べないことを保証します。
  3. 飲み込まれたものは注意深く記録されます（event-log、log4net、YMMV）

通常、私はすべて、キャッチされなかった例外をCLRを終了することによってアプリケーションを単に「クラッシュ」させる練習をしています。ただし、特にサーバーアプリケーションでは、これが適切でない場合があります。1つのスレッドで致命的ではないと見なされる問題が発生した場合、プロセス全体をリッピングして、実行中の他のすべての要求を強制終了する理由はありません（たとえば、WCFはこの方法でいくつかのケースを処理します）。

于 2009-01-10T17:34:06.543 に答える
2

私が見たほとんどすべての Java / C# コードの例外処理が正しくないため、ここに追加したいと思います。つまり、無視された例外のエラーをデバッグするのが非常に困難になるか、または同様に悪いことに、何も言わないあいまいな例外が発生します。

まず、例外は、コード レイヤー間でエラー情報を簡単に返す方法であることを理解してください。ここで、間違い 1: レイヤーは単なるスタック フレームではなく、明確に定義された責任を持つコードです。という理由だけでインターフェイスと impl をコーディングしただけなら、もっと良い修正が必要です。

レイヤーが適切に設計されていて、特定の責任がある場合、エラーの情報はバブルアップするため、異なる意味を持ちます。<-これが何をすべきかの鍵であり、普遍的なルールはありません。

したがって、これは、例外が発生したときに 2 つのオプションがあることを意味しますが、レイヤーのどこにいるのかを理解する必要があります。

A) レイヤーの途中にいて、内部の、通常はプライベートなヘルパー関数で、何か問題が発生した場合: 心配しないで、呼び出し元に例外を受け取ってもらいます。ビジネスコンテキストがなく、1) エラーを無視していないため、完全に問題ありません。2) 呼び出し元はレイヤーの一部であり、これが発生する可能性があることを知っている必要がありますが、下でそれを処理するコンテキストがない可能性があります。

また ...

B) あなたはレイヤーの最上部の境界であり、内部へのファサードです。次に、例外が発生した場合、デフォルトは CATCH ALL になり、呼び出し元にとって意味のない特定の例外が上位層に渡らないようにするか、さらに悪いことに、変更する可能性があり、呼び出し元は実装に依存します。詳細と両方が壊れます。

アプリケーションの強みは、レイヤー間の分離レベルです。ここでは、一般的な規則としてすべてを停止し、上位層にとってより意味のあるエラーに情報を変換する一般的な例外でエラーを再スローします。

ルール: レイヤーへのすべてのエントリ ポイントは CATCH ALL で保護され、すべてのエラーは変換または処理されます。現在、この「処理済み」は 1% の確率でしか発生しません。ほとんどの場合、正しい抽象化でエラーを返す必要がある (または返すことができる) だけです。

いいえ、これを理解するのは非常に難しいと思います。実際の例 ->

いくつかのシミュレーションを実行するパッケージがあります。これらのシミュレーションはテキスト スクリプトです。これらのスクリプトをコンパイルするパッケージがあり、テキスト ファイルと、もちろんベース Java RTL を読み取るだけの汎用 utils パッケージがあります。UML 依存関係は->

Simulator->Compiler->utilsTextLoader->Java File

1) 1 つのプライベート内の utils ローダーで何かが壊れて、FileNotFound や Permissions などを取得した場合は、それを通過させます。他にできることは何もありません。

2) 境界では、最初に呼び出される utilsTextLoader 関数で、上記のルールと CATCH_ALL に従います。コンパイラは、ファイルがロードされたかどうかに関係なく、何が起こるかを気にしません。したがって、キャッチでは、新しい例外を再スローし、FileNotFound などを「ファイル XXXX を読み取れませんでした」に変換します。

3) コンパイラは、ソースがロードされていないことを認識します。それはそれが知る必要があるすべてです。したがって、後でネットワークからロードするように utilsTestLoader を変更しても、コンパイラは変更されません。FileNotFound を手放して後で変更すると、コンパイラに何の影響もありません。

4) サイクルが繰り返されます。ファイルの下位レイヤーを呼び出した実際の関数は、例外を取得しても何もしません。だからそれは上に行くことができます。

5) 例外がシミュレーターとコンパイラーの間のレイヤーに到達すると、コンパイラーは再び CATCHES_ALL を返し、詳細を非表示にして、より具体的なエラーをスローします:「スクリプト XXX をコンパイルできませんでした」

6) 最後に、このサイクルをもう一度繰り返します。コンパイラーを呼び出したシミュレーター関数は手放します。

7) 最終的な境界はユーザーにあります。ユーザーはLAYERであり、すべてが適用されます。メインには catches_ALL という try があり、最終的には適切なダイアログ ボックスまたはページを作成し、翻訳されたエラーをユーザーに "スロー" します。

だからユーザーは見ます。

シミュレータ: 致命的なエラーでシミュレータを起動できませんでした

-コンパイラ: スクリプト FOO1 をコンパイルできませんでした

--TextLoader: ファイル foo1.scp を読み取れませんでした

---trl: ファイルが見つかりません

比較:

a) コンパイラ: NullPointer Exception <-一般的なケースと、ファイル名のタイプミスをデバッグするための失われた夜

b) ローダー: ファイルが見つかりません <- ローダーが何百ものスクリプトをロードすることについて言及しましたか??

また

c) すべてが無視されたため、何も起こりません!!!

もちろん、これは、再スローのたびに原因例外を設定することを忘れていないことを前提としています。

さて、私の2cts。このシンプルなルールが私の命を何度も救ってくれました...

-エール

于 2013-01-10T01:53:28.143 に答える
1

通常はグローバル ハンドラ (予期しない例外をログに記録するのに最適な場所) で一般的な例外をキャッチする必要がありますが、それ以外の場合は、前に述べたように、何かを行う予定がある場合は、他の場所で特定の例外タイプのみをキャッチする必要があります。catch ブロックは、投稿したコードのようにではなく、これらの例外の種類を明示的に探す必要があります。

于 2009-01-08T23:20:55.477 に答える
0

プリンシプルは、処理できる例外をキャッチすることだけです。たとえば、findnotfound の処理方法を知っている場合は filenotfoundexception をキャッチし、それ以外の場合はキャッチせずに上位層にスローします。

于 2013-01-16T07:00:06.840 に答える
0

この場合、これはそれほど悪いことではないと思いますが、コードで同様のことを行い、安全に無視できる例外をキャッチして残りを再スローします。Michael's responseで指摘されているように、各キャッチを個別のブロックにすることで、この方法を使用することで回避できる読みやすさの問題が発生する可能性があります。

これを同僚に指摘することに関しては、これが間違ったやり方であると彼らに納得させるのは難しいと思います。彼らが頑固な場合はなおさらです。他の方法で物事を行うと読みやすさの問題が生じる可能性があるためです。 . このバージョンはまだ処理できない一般的な例外をスローしているため、プラクティスの精神に沿っています。ただし、コードが次のとおりである場合:

try
{
  // Do some work
}
catch (Exception ex)
{
  if (ex is SoapException)
  {
    // SoapException specific recovery actions
  }
  else if (ex is HttpException)
  {
    // HttpException specific recovery actions
  }
  else if (ex is WebException)
  {
    // WebException specific recoery actions
  }
  else
  {
    throw;
  }
}

次に、一般的な例外ブロックで特定の例外をチェックして特定の例外の作業を行う意味がないため、心配する理由がもう少しあると思います。

于 2009-01-09T13:31:13.273 に答える