はじめに
過去にSonarCloudを利用して、検出をされた改善点・問題に対する対応の記事をまとめてみましたが、私のプロジェクトでの運用から検出ルールから除外しても良いルールやIssueの対応について再度まとめてみようと思います。
私のチームで除外しても良いと判断したルールとその理由
Boolean literals should not be redundant
読みやすさを向上させるために、冗長なブール リテラルを式から削除する必要があります。
というルールですが、bool値をそのまま利用した判定でなく、追加で比較処理などしている箇所で検出されます。例えばif(boolMethod() == true)
のような利用箇所などです。
私のチームでは、意図的に可読性を高めるためにbool型を== false
で判定した記述をする場合があります。この場合は、今度のメンテナンス性や振る舞いの可読性がこの記述のほうが高いという判断の元、行っており、その箇所で毎度Issueを検出されて欲しくないため検出除外としました。
(前提として、冗長な判定を基本的に記述しないチームのため、本来このルールで検出したい箇所がないという場合に限るかもしれません)
"Find" method should be used instead of the "FirstOrDefault" extension
FirstOrDefaultメソッドではなくてFindメソッドを利用したほうが高速になる可能性があると警告されています。
私達のチームではわずかなパフォーマンスの高速化よりもNullアクセスといったエラーが発生することのほうがユーザーに対する不利益が大きいと捉え、なるべくフェールセーフな実装を行う過程でデフォルト値を返すFirstOrDefaultメソッドを利用することが多々あります。そのため、意図的にFindよりもFirstOrDefaultを利用しており検出ルールから除外するとしました。
Methods should not have too many parameters
長いパラメータリストを持つメソッドは、各パラメータの役割を理解し、その位置を追跡しなければならないため、使用するのが困難であり、デフォルト設定では8個以上の引数を指定するようなメソッドに対して検出されます(コンストラクタには検出されません)
私のチームでも基本的にはそのような複雑なメソッドは作成しないため検出されませんが、これが検出されたのがアプリケーションの拡張ポイント(リボンに新しいメニューを追加する場合に名前やアイコン、説明、押下したときのアクション・・・)をまとめて登録できる箇所で検出されていました。これは登録口が分かれると利用しにくいため、意図的に多くの引数を要求している箇所ですし、このような設計パターンをよく利用するので、毎度Issueを検出されて欲しくないため検出除外としました。
まとめ
基本的には検出された指摘というのはなんらかコードをより良くしてくれる参考であるため、取り入れていくのが良いですが、チームの成熟度や方針を踏まえた上で合わないルールというのはいくつか出てくるはずです。
ルールとして除外するのではなくて、検出された箇所に対して次から検出されないように設定することも可能です。十分熟慮した上でチームとして除外するルールを定め、本当に指摘してほしかったものが埋もれないようにメンテナンスしていきましょう。