はじめに
Sonarcloudを利用してみて、導入したリポジトリにいろいろと改善点があるぞと教えてくれたので、
どんな問題が出ていて、どう対応するかまとめてみました。
SonarCloudのIssueについて
SonarCloudを導入してスキャンをかけた結果のうち、Issueタブに指摘が表示されます。
SonarcloudのIssue対応の設定は以下のように行う。
指摘された直後の場合はOpendと表示されている箇所をクリックすると画像のような選択が行える。
カーソルを合わせた際のツールチップを翻訳すると下記のような設定項目となっている。
- Confirm
この問題は検討されており、最終的には何か対処しなければなりません。 - Resolve as fixed
この問題はコード上で修正されており、次の解析で解決されるのを待っています。
実際には修正されていなかった場合は再オープンされます。 - Resolve as false positive
この問題は、解析エンジンの限界によるものなので、無視しても構いません。その労力はカウントされません。 - Resolve as wont't fix
この問題は、そのルールがこの文脈では無関係であるため、無視することができます。その努力はカウントされません。
では指摘が実際にどの選択肢に当てはまるのかは1つ1つ確認していかなければなりません。
とは言っても難しいことではなく、指摘の末尾にある**Why is this an issue?**をクリックすれば、
詳細な指摘の説明が出てくるため、その説明と実コードを見比べれば何をどうすべきかわかります。
たとえば以下は、「このコメントアウトされたコードを削除します。」と指摘されており
**Why is this an issue?**より詳細を読むとなぜ問題となっているのか教えてくれます。
「プログラマーは、プログラムが肥大化し、可読性が低下するため、コードをコメントアウトしてはいけません。
使われていないコードは削除すべきであり、必要であればソースコントロールの履歴から検索することができます。」
この内容を踏まえて、実際に修正すべきかどうか判断すれば良いだけです。
例：SonarCloudで指摘された内容
導入したリポジトリはLoC1000ちょっとの大きなものではないですが、結構指摘されていたので参考としてどんな問題が検出されるのか紹介します。
Update this implementation of 'ISerializable' to conform to the recommended serialization pattern.
ExceptionクラスがISerializableを継承しており、system.serializable属性をつけた方がいいと指摘されている。
dotnetのExceptionクラスを継承したクラスでもSystem.Serializable属性がついていないものもあり、実際にその属性が不要であることが多いため今回の検出箇所では対応不要と判断した。
⇨Resolve as wont't fixを設定
All 'Read' method overloads should be adjacent.
オーバーロードが隣接していません。
ファイルパス引数のReadメソッドとStream引数のReadメソッドが離れており指摘されてる。
後々追加したオーバーロードのメソッドが隣接していなかったため修正した。
⇨修正した後にResolve as fixedを設定
Remove this commented out code.
不要なコメントは削除すべき。
一部の実装箇所を現在は作りこんでおらず、そこをコメントアウトしており指摘されてる。
⇨現在は作りこんでいないためConfirmとして指摘を残すようにしている。
Change the visibility of this constructor to 'protected'.
アクセス修飾子をpublicからprotectedにできると指摘されてる。
⇨その通りであったため修正した後にResolve as fixedを設定
Refactor 'Issues' into a method, properties should not copy collections.
とあるクラスのIssuesというプロパティのゲッタでToListして値を返しているが、ToListは単純なフィールドアクセスより遅くなるためユーザがパフォーマンスの低さに驚かないようにリファクタリングすべきと指摘されている。
⇨ToListとせずにそのままリストを返しても問題なかったため、修正した後にResolve as fixedを設定
'System.Exception' should not be thrown by user code.
'System.Exception'は、ユーザーコードがスローしてはならないと指摘されている。
標準のExceptionをユーザがスローするとユーザーコードから意図して例外をスローしているのか、意図せず例外が起きているのかがわからなくなってしまうため修正が必要。
⇨ユーザ定義の例外クラスを利用するように修正した後にResolve as fixedを設定
最後に
見落としがちなところを自動で検出してくれるのでありがたいですね。
他にもあれば備忘録もかねて追記していきます。
2021/12/22 追記
Remove this 'base' call to 'object.GetHashCode', which is directly based on the object reference.
「object」を直接拡張するクラスは、「GetHashCode」または「Equals」の「base」の呼び出しを行ってはならないと指摘されている。
IEquatableを継承してEqualsおよびGetHashCodeをoverrideする場合、なんらかユニークなIDなどを利用してのロジックに変えたいという目的があるはずですが、overrideの実装でbase.Equalsなどと記述していては等価なはずなのに別として認識されてしまいます。
明確な不具合につながる部分なので修正が必要です。
Remove this conditional structure or edit its code blocks so that they're not all the same.
同じ実装のswitchまたはifチェーンにすべての同じ処理があるためエラーだと指摘されている。
コピペによるエラーかもと助言もしてくれていて、間違った処理であっても正常のように振る舞い不具合の原因となるため別処理を返すように修正が必要です。