Amazon CodeGuruを用いて悪しきコードを撲滅したい

ターゲット

• アプリケーション開発でコードを書いたことがあるひと
• コードレビューしてもらう相手（十分な技術・知識を備えた相手）がいないor 手が空いていない
• コードレビューをしたくても時間の確保が難しい

はじめに

昨今、従来からあるウォータフォール開発に始まり、アジャイル開発やプロトタイピング開発などソフトウェア開発手法が体系化されたことで、開発スピードとプロダクトの質ともに向上する動きが活発となっています。
しかし、開発スピードとプロダクトの質を担保しようとする際に必ずネックとなるものがあります。

それがコードレビューです。

コードを書いたことがある人は、個人の癖や経験が浅いときに誰しも悪しき習慣がコードに現れてしまう経験があると思います。

そこで、最良なコードレビューと悪しきコード撲滅する方法を考えたいと思います。

開発プロセスの体系化とコードレビューの関係

一般的なソフトウェア開発では各工程の粒度を如何にせよ、以下のプロセスを循環させます。
コーディング → ビルド + テスト → デプロイ → 計測 → 改善
開発するシステム規模が大きくなればなるほど、携わるエンジニア数は増大し、コード量も増加します。

コードレビューは機能がロジック上、正しく実装されているか、不具合はないか、そしてコード標準化・統一化を図ることで、システムのクオリティ向上（品質保証）することができます。

したがって大規模なソフトウェア開発においてコードレビューを行なうには、「規模相応に優秀なエンジニアの数が求められる」もしくは、「コードレビューを実施できるタスクコストを十分に確保しておく」必要があります。

最近ではCI/CD（継続的インテグレーション/継続的デリバリー）導入により、ソースコードを早期チェックし、バグ解決数の向上やコードレビュー工数を削減する動きがありますが、コード量が減るわけではないので抜本的な解決にはなっていません。

エンジニアの皆さんは、時間的コスト、タスク過多が及ぼす精神コストをなんとか削減したい。そう思うはずです。

そこでAmazon CodeGuru

Amazon CodeGuruとは、機械学習（ML）を活用して推奨事項を提供し、ソースコード品質の向上およびアプリケーションの最適化、負荷原因となるコード行の特定などを行なってくれるサービスです。re:Invent2019（基調講演の様子（動画；7分程度））でリリースされ、対応言語はJavaとPythonです（21年10月現在）。

サービスは、大きく次の2つに分かれています。

• Amazon CodeGuru Reviewer
  機械学習および自動推論を使用し、アプリケーション開発において本番環境にて問題発生前にコードの問題を特定したり、セキュリティの脆弱性を修正するよう修正箇所に対する推奨項目を提示する。
• Amazon CodeGuru Profiler
  アプリケーションのランタイムデータを分析し、CPUとメモリの使用率を視覚化し、パフォーマンス上の問題をトラブルシューティングすることでレイテンシーとスループットを改善します。
  また実行コストの大きなコードを視覚化し、開発者は優先順位をつけた修復が可能。

今回は本番運用前のソフトウェア開発において、コードの保守性を高めるため、Amazon CodeGuru Reviewerに注目して、自動コードレビューの有効性に着目します。

Amazon CodeGuru Reviewer

Amazon CodeGuru Reviewerは、機械学習と自動推論、AWS とセキュリティのベストプラクティス、および何千ものオープンソースと Amazon リポジトリに対する何百万ものコードレビューを通じて苦労して習得した教訓を使用して、コードレビューを自動化します。（公式ドキュメントより引用）
コードリポジトリは、GitHub、GitHub Enterprise、Bitbucket、または AWS CodeCommitに対応しています。

開発者は以下の2通りでコードレビューを実施することができます。

• フルリポジトリ分析
  分析を行ないたいリポジトリをCodeGuruに関連づけ、ソースブランチ単位で分析をかけられます。
• 増分コードレビュー
  PullRequestベースで分析を実行し、変更による増分コードに対して推奨事項がコメントされます。
  開発者はリポジトリの関連付けを一度行なうだけで、レビューに向けて都度特別な操作は不要です。

既存のコードレビュー自動化サービスとの比較

CodeGuruが発表される以前から存在するコードレビュー自動化サービスにSiderがあります。
Siderは日本産自動コードレビューサービスで、以下の特徴があります。

• UIや解析結果を日本語表示できる。
• コード解析以外の機能はない。（その分、解析速度は早い）
• 対応言語が多い。
  Ruby、PHP、JavaScript、TypeScript、CSS、Java、Kotlin、Go、Python、Swift、
  C/C++、C#、Shell Script、Dockerfile、Markdown etc.（21年10月現在）
• 既存分析ツール・独自開発ツールを組み合わせ、統合的なサービスとしてリリースしている。
  e.g.) Java→ PMD Java, Checkstyle, JavaSee（独自）

Amazon CodeGuruとSiderの特徴を以下にまとめました。（21年10月現在）

特徴	Amazon CodeGuru	Sider
対応言語	Java, Python のみ	Java, Pythonに加え、Ruby, Goなど15言語以上
対応リポジトリ	AWS CodeCommit, Bitbucket, GitHub, Github Enterprise Server	GitHub, GitLab
解析速度	△	〇
日本語対応	△	〇
仕組み	機械学習（Amazonリポジトリに対する数百万コードデビューデータ）	静的ルールセット、静的コード解析

世の中に蔓延る悪しきコード

ここで本題です。冒頭でも述べた通り、コードを書いたことがある人は、個人の癖や経験が浅いときに誰しも悪しき習慣がコードに現れてしまう経験があると思います。

そこで今回は一般的に指摘された経験があるであろう悪しきコードとして、

• 分かりにくい変数名
• 不適切なreturnの位置
• try-finallyでのリソースクローズ
• API-Keyべた書き
• MemoryLeak

を取り上げて、Amazon CodeGuruにコードレビューしてもらいます。

分かりにくい変数名

// bad
String str;  // 文字列であることしかわからない
String code; // 何のコードかわからない

int a;       // 何を指しているかわからない

File file1;	 //連番
File file2;

static final String MSGID_E0001 = "E0001"; // 定数名に値が入っている

private boolean writeFlg = false; // どういう場合にtrue/falseとなるのかが不明瞭
// good
String userName;
String messageCode;

int age;

File userListFile;
File companyListFile;

static final String MSGID_FILE_NOT_FOUND = "E0001";

private boolean writable = false;

不適切なreturnの位置

// bad
boolean isPrimeNumber(int num) {
    boolean ret;
    if (num < 2) {
        ret = false; // 2未満は素数でない
    } else if (num == 2) {
        ret = true;  // 2は素数
    } else if (num % 2 == 0) {
        ret = false; // 2以外の偶数は素数でない
    } else {
        ret = true; // 割り切れなかったら素数
        double sqrtNum = Math.sqrt(num);
        for (int i = 3; i <= sqrtNum; i+=2) {
            if (num % i == 0) {
                ret = false;   // 割り切れたら素数でない
                break;
            }
        }
    }
    return ret; 
}
// good
boolean isPrimeNumber(int num) {
    if (num < 2) {
        return false; // 2未満は素数でない
    }
    if (num == 2) {
        return true;  // 2は素数
    }
    if (num % 2 == 0) {
        return false; // 2以外の偶数は素数でない
    }
    double sqrtNum = Math.sqrt(num);
    for (int i = 3; i <= sqrtNum; i+=2) {
        if(num % i == 0) {
            return false;   // 割り切れたら素数でない
        }
    }
    return true; // 割り切れなかったら素数
}

try-finally

tryブロックの中で何らかのリソース（InputStream、OutputStream、BufferedReader等）を扱う場合、Java7以前はfinallyブロックでcloseメソッドを呼び出すことで、tryブロック内の処理が正常終了したか異常終了したかに関わらずリソースが確実に閉じられることを保証していました。

// bad
protected void tryFinallyCopy(String src, String dest) throws IOException {
	  String inFilePath = "D:\\foo.txt";
      String outFilePath = "D:\\bar.txt";

      FileInputStream in = null;
      FileOutputStream out = null;

      try {
          in = new FileInputStream(inFilePath);
          out = new FileOutputStream(outFilePath);
          int c;

          // データコピー
          while ((c = in.read()) != -1) {
              out.write(c);
          }
      } catch (IOException e) {
          e.printStackTrace();
      } finally {
          if (in != null) {
              try {
                  in.close();	// 入力ストリームを閉じて、関連システムリソースを解放
              } catch (IOException e) {
                  e.printStackTrace();
              }
          }
          if (out != null) {
              try {
                  out.close();	// 出力ストリームを閉じ、関連システムリソースを解放
              } catch (IOException e) {
                  e.printStackTrace();
              }
          }
      }
  }

問題点

• close処理を色々な箇所で行ったりリソースのnullチェックを行ったり等コードが冗長
• tryブロックの中とfinallyブロックの中の両方で例外が発生した場合、finallyブロックの中で発生した例外がスローされる（本来tryブロックの中で発生した例外が知りたいはずだが、その例外は抑制される）

こういった問題に対して、Java7でtry-with-resources構文が導入されました。try-with-resources構文によりこれらの問題は一挙に解決されます。

// good
protected void tryWithResourceCopy(String src, String dest) throws IOException {
	  String inFilePath = "D:\\foo.txt";
      String outFilePath = "D:\\bar.txt";

      // tryブロックを抜けるときにリソースを解放
      try (FileInputStream in = new FileInputStream(inFilePath);
              FileOutputStream out = new FileOutputStream(outFilePath);) {
          int c;

          // データコピー
          while ((c = in.read()) != -1) {
              out.write(c);
          }
      } catch (IOException e) {
          e.printStackTrace();
      }
  }

API-Keyべた書き

API-Keyをプログラムにべた書きしてしまっていることで、ハードコーティングされてしまい、他のプログラム間で共有する際にすべてのプログラムを書き換える必要があります。
また、セキュリティ的観点からもプログラムに記述するのではなく、環境変数やロールを活用した運用が必要です。

import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.BasicAWSCredentials;

static String myKeyId ="AKIAX742FUDUQXXXXXXX";
static String mySecretKey = "MySecretKey";

public static void main(String[] args) {
    AWSCredentials creds = getCreds(myKeyId, mySecretKey);
}

static AWSCredentials getCreds(String id, String key) {
    return new BasicAWSCredentials(id, key);
}

MemoryLeak

load()メソッドでテキストファイルの行をバッファし、printNextLine()メソッドで行数付の行内容を出力するもの。
しかし、このクラスを複数のファイル出力に使いまわそうとすると、listオブジェクトがload時に初期化されていないため、GCはlistに含まれる各行を表すStringを到達可能と判断し、メモリリークが発生してしまいます。

import java.util.List;
import java.util.LinkedList;
import java.io.File;
import java.io.OutputStream;
import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
 
public class MemoryLeakTest {
    private List list = new LinkedList();
    private int index;
    public void load(File file) throws IOException {
        index = list.size();
        BufferedReader reader = new BufferedReader(new FileReader(file));
        String buf = reader.readLine();
        while(buf != null) {
           list.add(buf);
           buf = reader.readLine();
        }
    }
 
    public int printNextLine(OutputStream os) throws IOException {
        if(index >= list.size())
            return -1;
        String line = (String)list.get(index++);
        int len = line.length();
        os.write((index + ":").getBytes());
        os.write(line.getBytes());
        os.write('n');
        return len;
    }
}

実際に分析をかけてみる

上記を踏まえ、リモートリポジトリとしてGitHubを利用し、Amazon CodeGuruに分析をかけてみます。今回は分析方法として、フルリポジトリ分析を行ない悪しきコードを指摘してくれるかどうかを確認します。

ファイル構成は次の通りです。

プログラム名	説明
BadPattern.java	変数名、return位置の悪い例をまとめたコード
GoodPattern.java	変数名、return位置の良い例をまとめたコード
MemoryLeakSample.java	メモリーリークが発生するコード
TryFinally.java	try-finallyでリソースを扱うコード
handleAPIKey.java	AWS API-Keyをべた書きしたコード

CodeGuru Reviewerを選択する

AWSマネジメントコンソールにアクセス、AmazonCodeGuruのダッシュボードへ進み、CodeGuru Reviewerを選択する。

リポジトリをAmazon CodeGuru Reviewerに関連付ける

今回はリモートリポジトリでGItHubを関連付けました。

GitHubアカウントの接続を終えると、以下のように✓マークが表示され、
「リポジトリの場所」にリポジトリ一覧から分析対象としたいものを選ぶ。

フルリポジトリ分析をかけてみる

CodeGuruによる分析結果

分析結果は以下のように得られました。
分析完了まではものの数秒で終えることができました。

要約すると以下の通りです。
handleAPIKey.java: 15行目
ハードコーティングした長期アクセスキーを用いず、一時的なセキュリティクレデンシャルであるIAMロールを使用してください。

MemoryLeakSample.java: 15行目
問題点
このコード行にはリソースリークが含まれている可能性があります。リソースリークはシステム速度の低下やクラッシュさせる恐れがあります。
修正点
readerをcloseすることを検討してみてください。readerがtry-finallyブロックで閉じられているか、try-with-resourcesブロックで宣言されていることを確認してください。

tryFinally.java: 11行目
このメソッドのサイクロマティック複雑度が11でCodeGuruの参照データセットに含まれるメソッドのうち98%がこのメソッドよりも低い値を示しています。
これはメソッド内に多数の条件分岐が存在し、ロジックの理解とテストが困難になる可能性を示しています。
メソッドを単純化するか、複数のメソッドに分割することをお勧めします。

以上から、大きく3つの指摘を受けることができました。

Siderとの比較

既存コードレビュー自動化サービスであるSiderを用いて、同じプログラムに分析をかけてみます。

詳細な設定方法は今回は割愛しますが、
CodeGuru同様にリモートリポジトリを連携することで容易に分析にかけることができます。
ブランチごとに分析をかけることができ、リポジトリごとに自動解析の有無を設定できるので便利です。

Siderによる分析結果

分析の結果、同じプログラムを用いて28個の指摘を受けることができました。

しかし、、、

そのほとんど（27/28個）が後述の通り、形式的に検出可能なものでした。
具体的には、

• パッケージを利用して変数を扱うこと
  https://pmd.github.io/pmd-6.39.0/pmd_rules_java_codestyle.html#nopackage
• privatefieldを使ってはならない
  https://pmd.github.io/pmd-6.39.0/pmd_rules_java_bestpractices.html#unusedprivatefield
• +=の前後に空白が入っていない
  https://checkstyle.org/config_whitespace.html#WhitespaceAround

など、すべてPMD Java, Checkstyleによる静的ルールセットに基づいて機械的に分析されたもので、CodeGuruが検出した、リソースクローズやAPI-Keyの件は現時点では検出することができない様です。（今後のアップデートや機能増強に期待したいですね。）

唯一想定していた指摘がされたものは、try-finallyを用いていることへの指摘でした。

分析結果を踏まえて

AmazonCodeGuruと既存サービスSiderそれぞれによる分析結果をまとめました。
まとめ方は、今回取り上げた悪しきコードに指摘があったかどうかで比較します。

Amazon CodeGuruによる指摘

悪しき習慣	指摘の有無	詳細
分かりにくい変数名	×	指摘無し
不適切なreturn位置	×	指摘無し
try-finallyでのリソースクローズ	〇	try-finallyに起因する問題には触れず、複雑度について指摘あり
API-Keyべた書き	◎	記述内容の指摘だけでなく、代替案としてawsリソースであるIAMロールを使用した方法を指摘された
MemoryLeak	◎	リソースリーク発生の可能性だけでなく、修正方法として具体的な記述法を指摘された

Siderによる指摘

悪しき習慣	指摘の有無	詳細
分かりにくい変数名	×	指摘無し
不適切なreturn位置	×	指摘無し
try-finallyでのリソースクローズ	◎	try-finallyをtry-with-resourcesに記述しなおすように指摘された
API-Keyべた書き	×	指摘無し
MemoryLeak	×	指摘無し

表に示す通り、特性上一長一短ですがAmazon CodeGuruのほうがソフトウェア開発における致命的なバグをさけ、かつセキュリティの脆弱性を考慮した指摘を得られることができたと思います。

おわりに

本記事では、ソフトウェア開発手法の体系化・多様化に伴い、開発スピードとプロダクトの質向上に最善を尽くすもののどうしてもネックになってしまうコードレビューに着目し、流行りの機械学習と組み合わせたサービスAmazon CodeGuruに触れてみました。

Amazon CodeGuruは指摘対象として、バグの発生やセキュリティの脆弱性、システム性能低下に繋がる重大な問題など、表面的に発見しづらいコードの問題点を発見し、改善案も含めて自動レビューしてくれることがわかりました。
一方で、変数名やアルゴリズムの実装方法など、属人的な悪しき習慣は指摘されないため、プロジェクト単位での緻密な統一や対人間のレビュー（お叱り）を行なう必要があるように感じました。

以上を踏まえ、最良なコードレビューとは、開発スピードとプロダクトの質向上を図るため、開発スピードを自動コードレビューによって担保し、品質を対人間レビューを補助的に行うことによって担保する という双方を組み合わせた運用が良いのではないかと思います。（いわゆるHuman-In-The-Loopな運用ですかね。）
また、悪しきコードを撲滅するには上記のように組織内での記述ルール統一や対人間レビューによるご指摘、そして自身が書いたコードに起因する失敗体験 が最も有効であるように思います。（あくまでも私個人の所感です。）

最後に、コードレビューはレビュアにとってコードリーディング技術の向上やエンジニア間のコミュニケーション向上などソフトウェア開発において品質特性の高いコード記述を実現するノウハウ共有という意味で重要な役割をもつので、人間がコーディングを行なう以上、完全にコードレビューを自動化してしまうのは控えたほうがよいのかもしれません。

参考

Amazon CodeGuru | 最もコストがかかるコード行の検出 | AWS
Sider - GitHub/GitLab対応のコードレビュー自動化サービス
Java初心者時代にコードレビューで指摘された悪しき習慣 - Qiita