LoginSignup
19
9

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

@gengen0719in株式会社Works Human Intelligence

SonarQubeさんに怒られたら

Last updated at Posted at 2024-06-10

まえがき

この記事に記載されている内容

SonarQubeは大変便利なツールです。
しかし解析結果の見方や検知されたIssueの内容が分からず無視してしまったり、
あまり理解せずにとりあえず直すという対応をしてしまったりしたことはないでしょうか？

この記事ではIssueの検知結果の確認にフォーカスして以下の内容を記載します。

  • SonarQubeの解析結果の確認の仕方
    • 検知されたIssueの表示のされ方
    • SonarQubeが提供してくれる情報の確認の仕方
  • 検知されたIssueがなぜ指摘されているのか調べる方法

SonarQubeさんに怒られたら、
めんどくさいなぁと思うのではなく
その内容を血肉に変えてレベルアップしましょう◎

SonarQubeとは

SonarSource社が開発する、様々な言語に対応したソースコードの品質管理ツールです。

ソースコードを静的解析し、以下のような処理を実施します。

  • バグの検知
  • 脆弱性の検知
  • 良くない記述の検知
  • コードの重複率の計測
  • テストカバレッジの計測
  • コードの複雑度の計測

など

解析した内容はWebコンソールに出力してくれるので、気軽に確認することができます。
CIに組み込んで、定期的に実行したり、PR(MR)の修正差分に対して実行することもできます。
解析した結果に閾値を設定し、基準に満たないコードのメインブランチへのマージを制限したりすることもできます。（Quality Gate）

SonarQubeを利用するメリット

解析結果がもたらすメリット

  • バグと脆弱性を未然に検知できることで外部品質の向上につながる
  • 良くない記述を検知できる、コードの重複率・テストカバレッジ・複雑度などを計測できることで内部品質の向上につながる

自動解析できることによるメリット

  • （人間だけが確認するよりも）見落としが減る
  • レビュアーの負担が減る
  • レビュアーに依存しない一定のフィードバックが得られる

その他のメリット

  • 解析結果が見やすい
    • Webコンソールで気軽に解析結果が確認できる
    • 修正差分の解析で自分が手を入れたコードの解析結果が見られる
    • Quality Gateで絶対対応しないといけないIssueがわかる

気を付けた方が良いこと

  • レビューが不要になるというわけではない
  • 指摘事項を修正した時にBugを発生させてしまう可能性もある（特にすでに運用されているコードの場合は注意）

参考

サンプルコードと解析結果の表示例

実行環境

この記事では私が保守開発を担当するプロダクトのSonarQubeに サンプルコードを解析させた結果を見ていきます。
以下のような実行環境になっています。

  • ソースコードの管理はGitLab
  • MR（マージリクエスト）のpipelineで修正差分に対する解析が行われる
  • 解析結果のサマリーはMRのOverViewにコメントとして記載される
  • OverViewコメントからSonarQubeのWebコンソールにアクセスできる

実行結果の表示は環境によって異なる場合があります。
抽出されるポイントやSonarQubeで扱う概念を押さえつつ、必要に応じて読み替えてください。

サンプルコード

SonarQubeさんに指摘される箇所をあえて含めたクラスを二つ用意しました。
これらのコードを新規追加クラスとしてMRを作成して指摘してもらいます。

その１　ConnectionNonClosedRepository
いかにも怒られそうなデータベースリソースをcloseしていないRepositoryクラスです。

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;

import javax.sql.DataSource;

import org.springframework.beans.factory.annotation.Autowired;

public class ConnectionNonClosedRepository {

	@Autowired
	DataSource dataSource;
	private static final String SQL = "UPDATE HOGE SET FUGA = ? WHERE ID = 1";

	public void save(String saveString) throws SQLException {
		Connection con = dataSource.getConnection();
		PreparedStatement ps = con.prepareStatement(SQL);
		ps.setString(1, saveString);
		ps.executeQuery();
	}

}

その２　DameDameClass
CodeSmellsの検知の実験のためだけに作られた悲しきモンスター。

import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;

public class DameDameClass {

	public void calc(int a, int b) throws SQLException {
		List<Integer> list = new ArrayList<>();
		list.add(new Integer(a));
		calcInner(list);
	}

	private void calcInner(List<Integer> list) {

	}

}

MRのOverviewに表示される内容を確認する

私の担当するプロダクトの環境の場合、以下のようにGitLabのMRにレポートが表示されます。12

  • ConnectionNonClosedRepositoryの解析結果

image.png

  • DameDameClassの解析結果

image.png

上から順に以下のような内容です。

  • Quality Gate
    一番最初に表示されるのがQuality Gateの状態です。
    ConnectionNonClosedRepository はFailedになっています。
    Failedの場合は重大な問題が検出されマージできない状態であることを表しています。
    DameDameClass はPassedになっています。
    Passedの場合はQuality Gateを通過しておりマージすることができる状態です。3
  • Additional information
    Quality GateのPassed,Failedに加えた詳細な情報が表示されます。
  • Issues
    全体でいくつの問題が検出されたのかを表しています。
    • Bugs
      多くの場合不具合になるような実装が検知された数です。
      ConnectionNonClosedRepositoryでは2件のBugsが検出されています。
    • Vulnerabilities
      明らかにセキュリティ上の問題となるような実装が検知された数です。
    • Security Hotspots
      状況によってはセキュリティ上の問題となるような実装が検知された数です。
    • Code Smells
      コードを読みづらくするような実装が検知された数です。
      DameDameClassでは4件のCode Smellsが検出されています。
  • Coverage
    修正コードの何パーセントがJUnitTestで実行されるのかを示した数値です。
  • Duplication
    修正コードの何パーセントが重複しているかを示した数値です。

コンソールに表示される内容を確認する

それぞれの抽出結果をクリックすることでSonarQubeのWebコンソールを開くことができます。コンソールのIssuesタブを見ると検知された問題のリストを確認することができ、Codeタブを見るとそれぞれのIssueがソースコードのどの箇所で発生しているのかを確認することができます。（一部リポジトリ名やコミッタの名前等を黒塗りしてます）

  • ConnectionNonClosedRepositoryのコンソール

image.png

ConnectionNonCloseRepository_Issues.png

ConnectionNonCloseRepository_Code.png
ConnectionとPreparedStatementをcloseしていないコードがBugsとして検出されているおり、finally節でcloseするように指示が書かれています。

  • DameDameClassのコンソール

image.png

Bugs.png

Source.png
利用されていない引数、throwされない例外のthrows節、Integerのコンストラクタ呼び出しがCode Smellsとして検出されており、それぞれ修正するように指示が書かれています。
各Issueの指摘内容を見て理解できるものは、修正、コミットして再提出しましょう。

指摘内容の詳細を確認する

指摘内容が理解できないものについては Why is this an issue? というリンクをクリックすることで説明が表示されます。

ConnectionNonClosedRepository の Use try-with-resources or close this "Connection" in a "finally" clause. の例

image.png

DameDameClass の Remove this "Integer" constructor の例

image.png

ここには以下のようなことが記載されています。

  • Noncompliant Code Example（良くないコードの例）
  • Compliant Solution（修正コードの例）
  • Exception（例外的に修正しなくても良い場合）

ConnectionNonClosedRepository の例であれば、提出コードのような単純にcloseが呼び出されていない問題以外でも検出される例が示されており、それぞれの解決方法や例外的にcloseを呼び出さなくても良い事例も示されています。
DameDameClass の例であれば、型ごとに良くない書き方と良い書き方が示されています。

執拗に調べて理解する

ここまでの内容を確認してまだ納得できないものがある場合は他の方法で調べてみることで、より理解が深まり血肉に変わっていきます。
DameDameClass の Remove this "Integer" constructor の解説ではIntegerクラスのインスタンスを作る際には valueOf よりも new を利用した方が多くのメモリが使用されると記載されていましたが、それは何故なのかを調べてみます。

公式ドキュメントを参照する

public static Integer valueOf(int i) の説明には以下のように記載されています。

指定されたint値を表すIntegerインスタンスを返します。新規Integerインスタンスが不要な場合、通常このメソッドがコンストラクタInteger(int)に優先して使用されます。その理由は、このメソッドが頻繁に要求される値をキャッシュするので、操作に必要な領域や時間がはるかに少なくて済む場合が多いためです。このメソッドは、-128から127の範囲(両端含む)の値を常にキャッシュしますが、この範囲に含まれないその他の値をキャッシュすることもあります。

なるほどvalueOfを利用するとキャッシュされているインスタンスを返すためコンストラクタを利用するよりも効率的であることがわかりました。
またその範囲が-128から127である（ただしこの範囲外の値がキャッシュされることもある）ことがわかりました。

生成AIに質問してみる

生成AIに質問してみると、おそらく公式ドキュメントを参照しているような答えが返ってきました。

質問
JavaのInteger型のインスタンスを生成する際にコンストラクタよりもvalueOfを利用した方が良いのは何故ですか？

JavaのInteger型のインスタンスを生成する際、
コンストラクタを使用する方法よりもvalueOfメソッドを使用した方が良い理由はいくつかあります。

1.パフォーマンスの向上：
valueOfメソッドは、事前にキャッシュされたIntegerオブジェクトを再利用することができます。
そのため同じ値を持つIntegerオブジェクトを複数回生成しても毎回新しいオブジェクトを作成する必要がありません。
一方、コンストラクタを使用すると、新しいオブジェクトが毎回生成されます。

2.メモリの節約：
valueOfメソッドを使用することで、Javaのインターンプール機構によってIntegerオブジェクトが再利用されるため、メモリの使用量を節約することができます。
コンストラクタを使用すると、新しいIntegerオブジェクトが毎回生成され、メモリ使用量が増えてしまいます。

3.コードの簡潔化：
valueOfメソッドを使用することで、コードが簡潔になります。
コンストラクタを使用する場合、値を入力するために毎回新しいIntegerオブジェクトを作成する必要がありますが、valueOfメソッドを使用する場合、入力値に合わせたIntegerオブジェクトが返されるため、コードの記述が簡潔になります。

ただし、注意点として、valueOfメソッドはキャッシュされたIntegerオブジェクトを生成できる範囲に限界があります。例えば、-128から127の範囲に限定している場合など、キャッシュされるIntegerオブジェクトの範囲に注意する必要があります。

回答をもらった後に「参照した情報を教えてください」と質問すると情報の精度も確認できて良いかもしれないです。

実装を覗く

常にここまでする必要はないと思いますが、実装を確認できる場合は実装を見てみることも有効だと思います。
公式ドキュメントの -128から127の範囲(両端含む)の値を常にキャッシュしますが、この範囲に含まれないその他の値をキャッシュすることもあります。 という説明が気になったため、今回は実装を覗いてみることにしました。

Integer.valueOfの実装は以下のようになっていました。

    public static Integer valueOf(int i) {
        if (i >= IntegerCache.low && i <= IntegerCache.high)
            return IntegerCache.cache[i + (-IntegerCache.low)];
        return new Integer(i);
    }

キャッシュの範囲内の場合はキャッシュから取得して返す、範囲外の場合は new して返すというような実装になっています。
またIntegerCacheは以下のような実装でした。

    private static class IntegerCache {
        static final int low = -128;
        static final int high;
        static final Integer cache[];

        static {
            // high value may be configured by property
            int h = 127;
            String integerCacheHighPropValue =
                sun.misc.VM.getSavedProperty("java.lang.Integer.IntegerCache.high");
            if (integerCacheHighPropValue != null) {
                try {
                    int i = parseInt(integerCacheHighPropValue);
                    i = Math.max(i, 127);
                    // Maximum array size is Integer.MAX_VALUE
                    h = Math.min(i, Integer.MAX_VALUE - (-low) -1);
                } catch( NumberFormatException nfe) {
                    // If the property cannot be parsed into an int, ignore it.
                }
            }
            high = h;

            cache = new Integer[(high - low) + 1];
            int j = low;
            for(int k = 0; k < cache.length; k++)
                cache[k] = new Integer(j++);

            // range [-128, 127] must be interned (JLS7 5.1.7)
            assert IntegerCache.high >= 127;
        }

        private IntegerCache() {}
    }

デフォルトでは-128～127の範囲がキャッシュされていることが確認でき、最大値はpropertyによって変わる場合もあることが分かりました。

実験をする

これも常に行う必要はないと思いますが、気になる実装があった場合は実験をしてみることで理解が深まることもあります。
キャッシュしていて同一インスタンスを返す、ということは == 演算子で比較した際に同一のオブジェクトの場合のみ true になるはずだということで、以下のようなコードを書いてみました。

	public static void main(String[] args) {
		Integer valueofInteger100 = Integer.valueOf(100);
		Integer valueofInteger100_2 = Integer.valueOf(100);

		Integer newInteger100 = new Integer(100);
		Integer newInteger100_2 = new Integer(100);

		System.out.println(valueofInteger100 == valueofInteger100_2);
		System.out.println(newInteger100 == newInteger100_2);

		Integer valueofInteger200 = Integer.valueOf(200);
		Integer valueofInteger200_2 = Integer.valueOf(200);

		Integer newInteger200 = new Integer(200);
		Integer newInteger200_2 = new Integer(200);

		System.out.println(valueofInteger200 == valueofInteger200_2);
		System.out.println(newInteger200 == newInteger200_2);
  
	}

実行結果は以下のようになりました。

true
false
false
false

キャッシュの範囲内の値でかつvalueOfで取得したインスタンスの場合は == で比較した時に true となっており同一のインスタンスが取得されていることが分かります。

Objectの値の比較を == で実施するというのはJavaの初学者の方がやってしまいがちな誤った実装ですが、IntegerクラスのvalueOfを利用した場合はキャッシュされている範囲内のみ正しく動いているように見えてしまうというさらに混乱させられる挙動であることが分かります。

あとがき

SonarQubeの結果確認から少し飛躍した内容になってしまいましたが、このSonarQubeの指摘はどういう意味なのだろうという疑問が湧いた時に自己解決する助けとなれば幸いです。
細かな言語仕様を理解しておくとトラブル対応時や不具合修正をする時に活きることがあるので、気になることがあったらとことん調べてみるのも良いのではないか思います。

おわり。

  1. 解析結果への導線は大変重要です。
    MRのOverviewに記載されることで見逃しなく、人によるレビューを受ける前に
    SonarQubeの指摘内容を確認することができます。 ↩

  2. 既に解析結果を確認できるプロダクトで開発している方は幸せです。
    実装してくださった方に感謝しながら、必ず解析結果を確認しましょう。 ↩

  3. PassedとFailedの境界はカスタマイズすることができます。
    SonarQubeの解析結果は明らかに不具合につながるようなものからできれば直しておいた方が良いというようなものまでさまざまです。
    そのためプロジェクトの状況によって厳しくしたり緩くしたりすることができます。 ↩

お題は不問！Qiita Engineer Festa 2024で記事投稿！
Qiita Engineer Festa20242024年7月17日まで開催中！
19
9
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
19
9

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?