あの、有名なTwitterクライアントのTweenのソースコードにVisual Studioが提供するコード分析機能で分析をかけてみました。あくまで、コードの奥深くまで踏み込んだ分析はしてないので、実際のコードを見ると、また違った印象を受けるかもしれませんが、Visual Studioのコード分析機能がどんなものかというのを試してみたいと思います。
まずはじめに
仕事柄、コードの品質を見てくれと言われることが多かったので、それなりにコード分析系のツールは使ってきたりしました。だって、そうしないと全部のコード見るわけには現実的には辛いですからね・・・。
ということで、自分がコードの分析を頼まれた時に、大体どんなことをしてたのかということも絡めながら紹介できたらと思います。
Tweenのソースコードは、以下のサイトから落とすことが出来ます。
因みに、ソースコードの分析結果をBlogに公開していいという許可は作者の kiri_featherさんに許可をいただいています。
Visual Studioのコード分析機能
さて、Visual Studioのお高いエディション(Premium以上)からついてくる機能なので、普通は、あまり縁のない機能だとは思いますが、コード分析は継続的に使ってると便利なツールだと思うので１つくらいあっても便利かもしれません・・・。
因みに、ここで言ってるコード分析は、コード分析機能とコードメトリックスの計算も含んでいます。
コード分析をかけてみよう
ということで早速TweenのプロジェクトをVisual Studioで開いてコード分析にかけてみようと思います。プロジェクトのプロパティからコード分析のタブを開いてルールセットでMicrosoftのすべての規則を選択します。そして、開くボタンを押してルールのカスタマイズをする画面を開きます。
因みに、ここでは細かいルールの解説はしません。細かいルールの内容を知りたい方は、以下のMSDN内のサイトを参考にしてください。
コード分析のルールをセットするときには、いらないルールを検討して最適なルールセットを作成する必要があります。この手順を踏まないと、コード分析の結果が膨大になりすぎて何がなんだかわからなくなったり、必要なチェックをスルーしたりする危険があります。こういうのは、会社で品質見てくれる部門や、プロジェクトでそういう役割を担ってる人が決めてくれてるとありがたいですね。
Tweenに適用したルールセット
Tweenは、既にうごいてる実績のあるコードなのでおそらくバグはそんなにボロボロあるようなことは無いと思います。なので、致命的なバグに繋がる可能性のあるもの、リソースリークのあるようなものに絞ってルールセットをカスタマイズしました。
超絞ったので、ほとんどルールは消してます…。
ルールを適用してみた
あれだけ絞り込んだルールセットでTweenのコード分析をかけてみると88件の警告としてルール違反が見つかりました。代表的に引っかかったルールを見てみます。
- CA1806 : メソッドの戻り値を無視ってる(15件)
- 普通は、メソッドの戻り値は有効活用しますよね。それが守られていないということは何か無視してる箇所にきな臭い感じを感じます。
- CA1816 : Dispose内でGC.SuppressFinalize(Object)を呼んで(2件)
- Disposeの実装パターンに従ってないということですね。なおすと若干・・・パフォーマンスが実感できない位に上がる可能性が無きにしも非ずな弱気な感じです。
- CA1901 : 64bit化すると動かないんじゃね？(11件)
- P/Invokeしてる箇所で検出されました。恐らく64bitになったタイミングで型のサイズが変わるから、変なことになっても知らんぞということみたいです。これからの64bitの時代を生き抜いていくには、ちょっと怖い警告ですね。
- CA2000 : IDisposable実装してるクラスのDisposeを呼んでない(38件)
- かなり多い感じがしますが、個人的には致命的なクラスのDispose漏れはないのかな・・・？とは思います。ただ、長時間動かしてると何かしらリソースを食いつぶす可能性があるかもしれません。
- CA2200 : 個人的に一番ひっかかると嬉しいルールです。例外が持ってるスタックトレースの情報を殺してエラーが発生したときの大切な情報を教えてくれない秘策です(6件)
- Throw exからThrowになおすだけなのでなおしておいたほうがいいと思います。
- CA2213 : IDisposableのフィールドを持ってるクラスにIDisposableがない(28件)
- これも2000と同じような感じです。すぐ止まるみたいなコードではないように見えました。
- CA2218 : Equalsを書き換えたのにGetHashCodeを再定義していない(1件)
- これは・・・Hashのキーになることはないクラスなのかな・・・？
ルールの結果をExcelに張ったものは以下からダウンロードできます。
メトリックスの計算
では、コードがどれくらいカオスなのか可視化してみようと思います。コードメトリックスを計算した結果はExcelに吐き出せるので吐き出してExcelで見るほうが簡単です。まず、スコープをメンバーにしてメソッド単位で情報が出るようにしましょう。そしてサイクロマティック複雑度でソードします。
そうすると、ちょっと驚くメソッドが出てきました。
| 型 | メンバー | 保守容易インデックス | サイクロマティック複雑度 |
| TweenMain | CommonKeyDown(Keys, TweenMain.ModifierState, TweenMain.ModifierState) As Boolean | 0 | 200 |
なんとこいつ単体でサイクロマティック複雑度200をたたき出しています。3ケタに届くようなメソッドはお見かけすることはありますが200はちょっと大杉ですね。これをテストしようと思ったら単純に考えても200ケース以上テストしないとテストできたことになりません。悩ましいですね。そして、保守容易インデックスなんですが、これは大きいほど保守しやすいということです。これが0という絶望的な数字になってます。ここらへんは、ちょっと手を入れたくない感じですね。
サイクロマティック複雑度
サイクロマティック複雑度ってなぁに？というかたは以下を見てください。
これは、一般的には１０を超えたあたりからリファクタリングしましょうと言われてたりします。ただ、現実的な話をすると25くらいから嫌な感じのコードになってくる感覚があります。この個人的な感覚でいくと見た感じ不快感を得ることが出来るメソッドはTweenにこれだけあることになります。
- 47メソッド/3902メソッド
どうやら１％ちょっと程不快感を与えてくれる逸材が潜んでいるようです。
ちなみにNo1を誇るメソッドは以下のようになっています。誌面の都合上拡大率20%で撮影した画像になります・・・
すごく・・・長いです・・・そしてインデントもいい感じに波打ってます。コードをざーっとスクロールしていって波を感じたらそのコードは危険です！ここまで高い波が来たら大人しく避難しましょう。
ちなみに、コードメトリックス計算の結果をExcelに吐き出したものは以下からダウンロードできます。
最後に
凄いと思ったのは、これだけ複雑なコードをかかえているのに、単体テストコードは０だということです。どうやって品質を保っているのか謎ですね。ということで以上！
