底辺プロジェクトの DoNotCatchGeneralExceptionTypes

前回普通で当たり前な例外処理コーディングについて書いたとき、同様のやり方が FxCop の開発者のブログ記事でも推奨されている事に触れた。実は、その記事の本来の趣旨は、例外を基底クラスの Exception型 で catch しようとする無知な輩を啓蒙するというものだった。

今回は、その事について考えてみる。ちょっとクドクドとネチっこく考えてみる。

＊以下、いちいち「例外を基底クラスの Exception型 で catch する」と書くのは面倒なので「基底キャッチ」と呼ぶ。

=＝＝＝
その記事で、件の開発者は以下のように告白している。

There are actually far too many catch (Exception) blocks in FxCop itself and it has caused us a great deal of grief. We’re working on removing them and I’m confident it will help us to deliver better software!

「実は、FxCop のソース自体に基底キャッチが濫用されていて酷い目にあったが、それを取り除いていくことで結局良いソフトウェアになった」と言う話。で、その教訓を踏まえて、基底キャッチがいかにダメな事なのか、基底キャッチ容認派への反論も交えて詳説している。

当の FxCop 開発者がそんな真似をしていたなんて意外だが、記事自体がもう4年半も前のものである事を考えると、相当に古い昔話なんだろうし、まあそんなものかと。でも、この記事やその他の同内容の言説のおかげで、基底キャッチが是か非かなんて問題を今さら蒸し返すような人は、少なくとも英語圏には少ないんじゃないかと思う。

====
ところが、未だに日本の底辺プロジェクトでは、基底キャッチで例外をもみ消して、「臭い物に蓋」よろしく無かった事にしてしまう手法が、当たり前のように日夜実践されている。しかも「教科書通りに行かないプロの現場で禁じ手を使いこなしてる俺って、やっぱプロだぜ！」てな感じで、一部の底辺プログラマが得意気な感じだったりもする。

そんなドン底の世界では、こんな事がもっともらしく力説される。

「うちのアプリは何があっても落とさないポリシーだから、どんな例外だろうとスルーは容認できない。防止も予測もできず、従って対処法の分からない未知の例外もあるだろうが、そう言うのはもみ消してでも続行させるしか手はない。それがうちの流儀なんだから仕方がない。」

で、全ての例外を漏らさないために、基底キャッチが導入されるというわけだが、まずそもそも、そんな想定外で正体不明な例外をもみ消したあと、アプリがまともに動くってどうして保証するんだろう？ DBに不正データを作ったり、誰かの給料を間違って計算したり、外部のサービスに不正なリクエストを送ったりしないと言えるのかと。そんなの技術以前に、論理的に矛盾してるし、言える訳がない。

そもそも顧客は何て言ってるんだろう。 「Null参照でアプリが落ちると嫌だから、取引先の公開サービスに不正リクエストを送っても良いから、動作続行させてください」なんて、言った人がいるんだろうか。でなければ、実行を続ける事で不測の重大事態が起きないと限らない事を伏せたまま、うちのアプリは落ちませんとだけ、顧客に説明している事になるわけで、それこそ大問題のはず。

====
エンドユーザの使用中には自分のアプリは落ちてほしくない。落ちるのは嫌だ。落ちるべきでない。それに敢えて異議を唱える訳ではない。が、だからと言ってアプリの誤動作の方がマシと言う事にもならない。だとしたら、いかに未知の例外によりアプリが停止する状況を極少化していくか、それしかないはず。

もうほとんど結論が出ているが、その前にもう少しだけ底辺プログラマの声に耳を傾けてみると、たいていこんな事を言っている。「発生し得る全ての例外の型をキャッチしていたら、いくつ catch ブロックがあっても足りない。現実的でない。」

この主張の駄目な点を指摘すると、以下の2つになる。

第一に、キャッチすべき例外が何か分かっていない。前回書いたフツーの例外処理の作法を分かっていれば、「全ての例外を…」なんて発想にはならない。発生の条件が既知であり、対象法がわかっている例外だけがキャッチする意味がある訳で、それ以外はキャッチせずにスルーしろと言っている。

第二は、第一のダメな点の原因でもあるのだが、やはりこれも「アプリを停止させるくらいなら、もみ消してしまいたい」という強迫観念に憑かれているという点。「アプリは停止できない」 → 「どんな例外もスルーできない」 → 「例外は須くキャッチすべき」という思考パターンだが、これが間違っているのは上で説明した。

予見できて対処できるものをキャッチするという、これこそ正に現実的としか言えない話しか、最初から誰もしていない。

====
これだけ言っても、まだ次のような言い分で食い下がる人もいる。

「そんな風に例外をスルーしてばかりいたら、その度にアプリが停止して、顧客がいちいち再起動する状況が頻発するから大問題になる。」

そんなの、例外とか catch の作法以前に、単に納品できる品質に至っていないという馬鹿な話でしかないんだが、実はこの辺りに問題の本質がある。

つまり、スルーした場合のアプリ停止にしても、もみ消した場合の不正動作にしても、それらは結果として出てきた現象なのでって、まず先んじて発生した未知の例外状況が、そもそもの原因としてある。

で、この「想定漏れ」や「考慮漏れ」、「テストから漏れたコーディングミス」などを含む原因側に働きかける事こそが、抜本対応と言えるわけだが、正にここにおいて、「例外スルー」と「例外もみ消し」は決定的に正反対の効果を見せてくる。

潔く例外をスルーする事は、結果としてアプリを落とす事になっても、結局は未知の例外状況の発生を減らしていく事になる。理由は、開発中に発見しやすく、従って現行犯的に処置できるため、もっとも効率がいいから。

一方、例外をもみ消す事とは、潜在バグを温存し溜め込んでいく行為に他ならない。健全な開発作業に当然含まれるはずの、意図しなかったのコードや想定から漏れていた条件を発見し、意図通りに直したり想定内に置き直していく営みが台無しになる。せっかく開発中に潰せたかもしれない未知の挙動を握りつぶして、客先まで不安定要因を持ち込んでしまっているわけだから、救いようがない。


かなりの極論になるが、突き詰めると、こういう選択になる。

数年に一回、未知の例外により停止するものの、その他は誤動作もない安定したアプリと、停止だけはしないが、年中不安定な動作を続けながら、しばしば深刻な誤動作をしでかし、しかもその障害解析が極めて困難なアプリとで、どちらが良いか？

使う側にとっても、作る側にとっても、どちらにとっても答えは決まってると思うんだが。

====
まあ、普通のプログラマは、分かり切った事なのに、ずいぶんくどいなあと思うかもしれない。自分だって「例外のもみ消しは、結局は品質落とすよね。」の一言で済む話だと、正直思ってた。そうした普通の感覚が成立しないどん底プロジェクトにいつ巻き込まれるか分からないから不景気って恐ろしい。

※ 私見だが、最初から checked と unchecked が峻別される Java の業界では比較的少なく、どちらかというと.NET 業界に多く見られる気がする。

C＃での例外処理コードの基本

C＃コーディングで例外をどう扱うか。

普通はこうなる。

<A.防止可能か？>
  ├─(yes)─→ [B.防止するコードを書く]
 (no)
  └→<C.予見可能か？>
       ├─(yes)─→ [D. catch して処理するコードを書く]
      (no)
        ↓
     [E. スルー]

A. 防止可能か？
例えば、「パラメータで null を渡されたら ArgumentNullException を投げる」ってドキュメントに書いてあったり、ソースコード上自明もの（バグも含む）だったり、例外発生の条件が分かっていて、そうならないようなコードを書ける場合は、防止可能と言う事になる。

B.防止するコード
A を満たす場合に、例外を発生させないようにコーディングする。

C.予見可能か？
防止できず、つまり実行するまで分からないが、状況によっては発生する事が分かっている例外状況を、「予見可能である」とする。例えば、ファイルを開くときの FileNotFoundException など。

D. catch して処理するコード
C で可能性が認められた例外については、catch してハンドリングするコードを書く。

E. スルー
防止も予見もできないものについては、復旧処理のしようも無いので、無理にアプリを動かし続けて誤動作による事態悪化を招くより、スルーしてアプリが停止するに任せる。書いてもせいぜい、AppDomain の UnhandleException イベントくらい。

この手順は、以下の URL で FxCop の開発者が詳説していたりもする。
http://blogs.msdn.com/b/codeanalysis/archive/2006/06/14/631923.aspx

まあ、普通のプログラマにとっては、いつも実践している通りの、言われるまでもない当たり前のコーディングじゃないだろうか。今更感満載で、「他にやり様あったっけ？ないでしょ？」てな感じになっちゃうと思う。

で、これを踏まえた上で、例えば、メソッドの API リファレンスに例外が沢山書いてあったらどうしようとか、catch 節もちゃんとテストしましょうだとか、finally を活用しましょうとか、いろいろな細かい事が生じてくる。

====
というのが、普通に計算機科学を勉強して、普通にプログラミングを習得して、普通にC＃を書いてる人の自然なコーディングプラクティスだと思うのだけど、底辺プロジェクトでは、この基本フローの段階で物言いがつく事がある。

曰く
「うちのアプリは何があっても落とさないポリシーだから、E のスルーは容認できない。未知の例外はもみ消して無かった事にするのが我々の流儀である。」と。

・・・いや、それ、マジで言ってんのかと。

次は、これについて考えてみる。

GUI コードのUnitTest の例

こんな感じのしょうもないフォームがあるとする。 DataGridView があり、A列と B列は数値を入力できる列で、C列は A - B の計算結果を表示する。 さて、このフォームには、C列の値が負のとき赤で表示するという振る舞いがあるが、果たしてこれをテスト駆動で書けるだろうか？ TDD に慣れた人なら、普通にテストファーストで書くだろうけど、ハナからできないと決め付けてる人も意外と多い。 そんな人が考えるテストはこんな感じ。

• アプリケーションを起動する。
• A列、B列に値を入力する。
• C列の前景色が変わったことを確認する。

まあ普通の手動の動作確認と同じだけど、これを自動化テストコードとしては書けないから UnitTest は無理と言う。 実際にはそういったテストコードも書けないことはないが、TDD の UnitTest ではそんなのは書く必要ないし、第一、テストの観点も目的も手法も違ってるから、むしろ書いてはいけない。 前に書いた HumbleView の作法でやると、例えばこんな風になる。

[TestFixture]
public class Form2PresenterTest
{
    public Form2Presenter presenter;
    public Mock<IForm2> mock;

    [SetUp]
    public void SetUp()
    {
        this.mock = new Mock<IForm2>();
        var view = mock.Object;
        this.presenter = new Form2Presenter(view);
    }
    [Test]
    public void SelectStyle_Plus()
    {
        TestSelectStyle(10, 5, CellStyle.NonNegative);
    }
    [Test]
    public void SelectStyle_Zero()
    {
        TestSelectStyle(10, 10, CellStyle.NonNegative);
    }
    [Test]
    public void SelectStyle_Minus()
    {
        TestSelectStyle(10, 20, CellStyle.Negative);
    }
    public void TestSelectStyle(int a, int b, CellStyle style)
    {
        //arrange
        var rowData = this.presenter.RowDataAt(0);
        rowData.A = a;
        rowData.B = b;
        //act
        this.presenter.Update(0);
        //assert
        mock.Verify(v => v.SetGridStyle(2, 0, style));
    }
}

見ての通り、Update()メソッドで行のインデクス（簡単のため0固定とした）を受け取った Presenter が、更新された A 列とB列の値に基づいてスタイルを選んで、セルの位置とともにビューに返すという流れになる。 これを満たす IForm2 と Presenter のコードはこんな感じ

public interface IForm2
{
    //･･･略･･･
    void GridDataSource(List list);
    void SetGridStyle(int col, int row, CellStyle styleId);
}

public class Form2Presenter
{
    //･･･略･･･
    public void Update(int rowIndex)
    {
        var rowData = this.list[rowIndex];
        var cellStyle = rowData.C < 0 ? 
                CellStyle.Negative : CellStyle.NonNegative;

        this.view.SetGridStyle(2, rowIndex, cellStyle);
    }
}

やり方はいくつかあるが、ここでは色を System.Drawing.Color で直接指定する代わりに、列挙値 CellStyle を定めて使っている。この CellStyle 定数と実際にセルに設定する DataGridViewCellStyle オブジェクトのマッピングを保持しておくクラスも別途必要だが、自明なので省略。 この辺りまで書くとテストも通る。 後は、View と Presenter を関連付けるだけ。

public partial class Form2 : Form, IForm2
{
    //…略…
    private void dataGridView2_CellEndEdit(
        object sender, DataGridViewCellEventArgs e)
    {
        this.presenter.Update(e.RowIndex);
    }
    public void SetGridStyle(int col, int row, CellStyle styleId)
    {
        this.dataGridView2[col, row].Style = CellStyleMap.StyleOf(styleId);
    }
}

前回と同様、View オブジェクト（Form2）は受身で丸投げなクラスで、余りテストの必要性が高くないほんの少量のコードを残して、その他の振る舞いのコードを Presenter に移した。 こんな風に、一見いかにも GUI の最表層みたいな振る舞いでも、フォーム＝View を薄く薄くしていく事で、妥当な網羅性を備えた UnitTest が書けるようになる。

C# のアンチイディオム

ソフトウェア開発の様々なテーマについて、それぞれパターンやイディオムやベストプラクティスがあるけど、まずはアンチパターンを避ける事が優先度が上で順序も先だと常々思う。

というわけで、書いてはいけない C#コードを『Effective C#』と『More Effective C#』から抜粋してみた（"m"が付いた番号が "More"の方）。

Item 09: Avoid Conversion Operators in Your APIs

• 変換演算子の定義は避ける事。

Item 26: Avoid Returning References to Internal Class Objects

• オブジェクトが保持している別のオブジェクトを外に晒すのは避ける事
• 特にコレクションなんかで、C#に限らず散々言われ続けてきた悪いコーディング

Item 16: Avoid Creating Unnecessary Objects

• 無駄なオブジェクトの生成を避ける事
• 文字列の構築での string の使用 ⇒ StringBuilder を使う事
• 高頻度で呼ばれるメソッド内でのオブジェクト生成 ⇒ メンバ変数や定数への格上げを検討する事

Item 32: Avoid ICloneable

• ICloneableの実装は避ける事
• パッと見の印象ほどには意外と使えないインターフェイスだったりする
• どうしても使うときは、基底クラスでコピーコンストラクタを定義した上で、sealed クラスのみで使う事。

Item 33: Use the new Modifier Only to React to Base Class Updates

• new モディファイアの使用は避ける事
• 外部のライブラリに含まれる基底クラスの変更により、それを継承している派生クラスで定義の衝突が生じるような稀なケースなどもあるが、そのときでもなるべく避ける事。

Item 34: Avoid Overloading Methods Defined in Base Classes

• 基底クラスで宣言されたメソッドをオーバーロードするのは避ける事
• こんなコードで、、、

  class B1 {}
  class D1: B1 {}
  class B2 { void Foo(D1 p) {...} }
  class D2: B2 { void Foo(B1 p) {...} }

  、、、D2.Foo() に D1 オブジェクトを渡すと、どっちが実行されるか意外と迷う。更に generics が絡むともっと面倒になる。

Item m07: Do Not Create Generic Specialization on Base Classes or Interfaces

• generic メソッドを特化するオーバロードは避ける事
• 例えば以下のようなコードで、、、

  class Base {}
  class Derived : Base {}
  class Foo {
     public static void DoSomething(Base b) { ... }
     public static void DoSomething(T b) { ... }
  }

  、、、DoSomething() に Derived インスタンスを渡すとどちらが実行されるか、意外と分かりにくい。DoSomething(Base b)を加える代わりに DoSomething(T b)の中で型チェックをすると、分かり易さも使い勝手も向上する。

Item m33: Avoid Modifying Bound Variables

• クロージャの中でのローカル変数の変更は避ける事
• 遅延実行絡みの分かりにくいエラーが生じやすい

Item m35: Never Overload Extension Methods

• 拡張メソッドのオーバーロードは避ける事
• 別々の namespace に異なる実装を書くと、拡張メソッドをオーバーロードした感じになるが、拡張メソッドの使い方としては間違い
• 別の識別子で別途定義すべき

Item m39: Avoid Throwing Exceptions in Functions and Actions

• シーケンスを操作する LINQ コードの中での例外送出は避ける事

Item m41: Avoid Capturing Expensive Resources

• クロージャがリソースをつかみっぱなしになるのを避ける事
• クロージャが、IDisposable なローカル変数を参照するコードを書くと、クロージャの生存期間や遅延実行がからんで面倒な事になりがち

Item m48: Avoid Calling Virtual Functions in Constructors

• コンストラクタで仮想関数を呼ばない事

Item m15: Avoid Calling Unknown Code in Locked Sections

• 未知のコードを lock ブロックの中で実行しない事
• 例えば、パラメータで渡されたりイベントに追加されたデリゲートや、オーバーライドされた仮想メソッドなどが、デッドロックを発生させる事がある

Item m24: Declare Only Nonvirtual Events

• virtual なイベントは避けること
• ほとんど意味が無く誰も得しない

Item m47: Limit Array Parameters to Params Arrays

• 可変引数以外では、配列を受け取るメソッドシグネーチャは避ける事
• covariance のせいで実行時に面倒くさいエラーが起こりがち
• シーケンスを渡すときは配列ではなく、IEnumerable を使う
• 中身を変更するときはIEnumerable を返す

中には FxCop とかで検出可能なものもあるらしいが、そうでないものも含めてコーディング規約とかガイドラインにも載せて、開発メンバで共有できるようにしたい。

C#で迷ったときのための備忘録

以下、『Effective C#』と『More Effective C#』から、
「A より Bを使いましょう」って感じで対比して書いてある項目を、抜書きしてみた。
Item NN が前者で C# 4.0、Item mNNが後者で C# 3.0。



Item 01: Use Properties Instead of Accessible Data Members
パブリック・フィールドよりプロパティを使いましょうという当然な話。

Item 02: Prefer readonly to const
runtime 定数と compile-time 定数の違い。極限られた場合を除いて、普通は前者を選択するに越したことは無いとのこと。

Item 03: Prefer the is or as Operators to Casts
安全かつ効率的という事で強く推奨。

Item 04: Use Conditional Attributes Instead of #if
Conditional 属性はメソッド単位でしかつけられないが、むしろそのおかげで良好な構造化が保て、#if/#endif の濫用による可読性低下を防止できる。

Item 08: Prefer Query Syntax to Loops
for/while などの手続型構文よりも、宣言型の Query 構文を使うべし。Linqのメソッドも場合により可。

Item 12: Prefer Member Initializers to Assignment Statements
ローカル変数の宣言と初期化と同じで、保守性が高い。

Item 22: Prefer Defining and Implementing Interfaces to Inheritance
昔からあるオブジェクト指向プログラミングのプラクティス。

Item 30: Prefer Overrides to Event Handlers
ちょっと意外だけど、どちらも使える状況でさえあれば、よく考えてみると Override の方が簡単な上に自由度が高いという話。

Item m09: Prefer Generic Tuples to Output and Ref Parameters
『More ～』の方なので、C#4.0のTupleでなく C#3.x 環境で、Generics を用いた自作のクラスの話題。今の携わってる案件がC#3.5なので、使える。

Item m11: Use the Thread Pool Instead of Creating Threads
.Net が提供してくれてる Thread Pool の方が、使いやすくて効率がいいとの事。

Item m22: Prefer Defining Methods to Overloading Operators
まあ、明らかにメソッドよりも演算子オーバーロードの記述性が高いなんて場面は、滅多にないしね。

Item m38: Prefer Lambda Expressions to Methods
ラムダ式の方が、より小さい要素に分解したり、それらを組み合わせたりするのに便利。関数型言語に馴染みがあれば断然読みやすいし。

Item m44: Prefer Storing Expression<> to Func<>
式を保存したり複雑に組み合わせたりするときは、FuncよりExpressionの方が便利という話。

にしても、前回からずいぶんと間が空いてしまった･･･