java-code-review

Java コードレビュースキル

Java プロジェクト向けの体系的なコードレビューチェックリストです。

使用するタイミング

• ユーザーが「このコードをレビューして」／「この PR を確認して」／「コードレビュー」と言ったとき
• PR をマージする前
• 機能を実装した後

レビュー戦略

1. クイックスキャン - 意図を把握し、対象範囲を特定します
2. チェックリストの確認 - 以下の各カテゴリを順に確認します
3. まとめ - 重要度順（重大 → 軽微）に指摘事項を列挙します

出力形式

## Code Review: [file/feature name]

### Critical
- [Issue description + line reference + suggestion]

### Improvements
- [Suggestion + rationale]

### Minor/Style
- [Nitpicks, optional improvements]

### Good Practices Observed
- [Positive feedback - important for morale]

---

レビューチェックリスト

1. null 安全性

確認項目：

// ❌ NPE risk
String name = user.getName().toUpperCase();

// ✅ Safe
String name = Optional.ofNullable(user.getName())
    .map(String::toUpperCase)
    .orElse("");

// ✅ Also safe (early return)
if (user.getName() == null) {
    return "";
}
return user.getName().toUpperCase();

問題となる点：

• null チェックなしのメソッド呼び出しの連鎖
• パブリック API に @Nullable / @NonNull アノテーションがない
• isPresent() の確認なしで Optional.get() を使用している
• Optional または空のコレクションを返せるメソッドが null を返している

提案：

• 値が存在しない場合がある戻り値の型には Optional を使用します
• コンストラクターやメソッドのパラメーターには Objects.requireNonNull() を使用します
• null ではなく空のコレクションを返します：Collections.emptyList()

2. 例外処理

確認項目：

// ❌ Swallowing exceptions
try {
    process();
} catch (Exception e) {
    // silently ignored
}

// ❌ Catching too broad
catch (Exception e) { }
catch (Throwable t) { }

// ❌ Losing stack trace
catch (IOException e) {
    throw new RuntimeException(e.getMessage());
}

// ✅ Proper handling
catch (IOException e) {
    log.error("Failed to process file: {}", filename, e);
    throw new ProcessingException("File processing failed", e);
}

問題となる点：

• 空の catch ブロック
• Exception や Throwable を広範囲に捕捉している
• 元の例外が失われている（チェーンされていない）
• フロー制御に例外を使用している
• チェック例外が API 境界を越えて漏れている

提案：

• コンテキストとスタックトレースを含めてログに記録します
• 具体的な例外型を使用します
• cause を使って例外をチェーンします
• ドメインエラーにはカスタム例外を検討します

3. コレクションとストリーム

確認項目：

// ❌ Modifying while iterating
for (Item item : items) {
    if (item.isExpired()) {
        items.remove(item);  // ConcurrentModificationException
    }
}

// ✅ Use removeIf
items.removeIf(Item::isExpired);

// ❌ Stream for simple operations
list.stream().forEach(System.out::println);

// ✅ Simple loop is cleaner
for (Item item : list) {
    System.out.println(item);
}

// ❌ Collecting to modify
List<String> names = users.stream()
    .map(User::getName)
    .collect(Collectors.toList());
names.add("extra");  // Might be immutable!

// ✅ Explicit mutable list
List<String> names = users.stream()
    .map(User::getName)
    .collect(Collectors.toCollection(ArrayList::new));

問題となる点：

• 反復処理中にコレクションを変更している
• 単純な操作にストリームを過剰に使用している
• Collectors.toList() が変更可能なリストを返すと想定している
• 不変コレクションに List.of()、Set.of()、Map.of() を使用していない
• 影響を理解せずに並列ストリームを使用している

提案：

• 防御的コピーには List.copyOf() を使用します
• イテレーターによる削除の代わりに removeIf() を使用します
• 変換にはストリーム、副作用のある処理にはループを使用します

4. 並行処理

確認項目：

// ❌ Not thread-safe
private Map<String, User> cache = new HashMap<>();

// ✅ Thread-safe
private Map<String, User> cache = new ConcurrentHashMap<>();

// ❌ Check-then-act race condition
if (!map.containsKey(key)) {
    map.put(key, computeValue());
}

// ✅ Atomic operation
map.computeIfAbsent(key, k -> computeValue());

// ❌ Double-checked locking (broken without volatile)
if (instance == null) {
    synchronized(this) {
        if (instance == null) {
            instance = new Instance();
        }
    }
}

問題となる点：

• 同期されていない共有可変状態
• アトミック性のない確認後実行パターン
• 共有変数に volatile がない
• final ではないオブジェクトをロックに使用している
• スレッドセーフではない遅延初期化

提案：

• 不変オブジェクトを優先します
• java.util.concurrent のクラスを使用します
• 単純なケースには AtomicReference、AtomicInteger を使用します
• @ThreadSafe / @NotThreadSafe アノテーションを検討します

5. Java の慣用表現

equals/hashCode：

// ❌ Only equals without hashCode
@Override
public boolean equals(Object o) { ... }
// Missing hashCode!

// ❌ Mutable fields in hashCode
@Override
public int hashCode() {
    return Objects.hash(id, mutableField);  // Breaks HashMap
}

// ✅ Use immutable fields, implement both
@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (!(o instanceof User user)) return false;
    return Objects.equals(id, user.id);
}

@Override
public int hashCode() {
    return Objects.hash(id);
}

toString：

// ❌ Missing - hard to debug
// No toString()

// ❌ Including sensitive data
return "User{password='" + password + "'}";

// ✅ Useful for debugging
@Override
public String toString() {
    return "User{id=" + id + ", name='" + name + "'}";
}

ビルダー：

// ✅ For classes with many optional parameters
User user = User.builder()
    .name("John")
    .email("[email protected]")
    .build();

問題となる点：

• hashCode を実装せずに equals を実装している
• hashCode で可変フィールドを使用している
• ドメインオブジェクトに toString がない
• パラメーターが 3～4 個を超えるコンストラクター（ビルダーを提案します）
• instanceof のパターンマッチング（Java 16+）を使用していない

6. リソース管理

確認項目：

// ❌ Resource leak
FileInputStream fis = new FileInputStream(file);
// ... might throw before close

// ✅ Try-with-resources
try (FileInputStream fis = new FileInputStream(file)) {
    // ...
}

// ❌ Multiple resources, wrong order
try (BufferedWriter writer = new BufferedWriter(new FileWriter(file))) {
    // FileWriter might not be closed if BufferedWriter fails
}

// ✅ Separate declarations
try (FileWriter fw = new FileWriter(file);
     BufferedWriter writer = new BufferedWriter(fw)) {
    // Both properly closed
}

問題となる点：

• Closeable / AutoCloseable に try-with-resources を使用していない
• リソースを開いているが、try-with-resources を使用していない
• データベース接続やステートメントが適切にクローズされていない

7. API 設計

確認項目：

// ❌ Boolean parameters
process(data, true, false);  // What do these mean?

// ✅ Use enums or builder
process(data, ProcessMode.ASYNC, ErrorHandling.STRICT);

// ❌ Returning null for "not found"
public User findById(Long id) {
    return users.get(id);  // null if not found
}

// ✅ Return Optional
public Optional<User> findById(Long id) {
    return Optional.ofNullable(users.get(id));
}

// ❌ Accepting null collections
public void process(List<Item> items) {
    if (items == null) items = Collections.emptyList();
}

// ✅ Require non-null, accept empty
public void process(List<Item> items) {
    Objects.requireNonNull(items, "items must not be null");
}

問題となる点：

• ブール値のパラメーター（列挙型を推奨します）
• パラメーターが 3 個を超えるメソッド（パラメーターオブジェクトを検討します）
• 類似メソッド間で null の扱いに一貫性がない
• パブリック API の入力値に検証がない

8. パフォーマンスに関する考慮事項

確認項目：

// ❌ String concatenation in loop
String result = "";
for (String s : strings) {
    result += s;  // Creates new String each iteration
}

// ✅ StringBuilder
StringBuilder sb = new StringBuilder();
for (String s : strings) {
    sb.append(s);
}

// ❌ Regex compilation in loop
for (String line : lines) {
    if (line.matches("pattern.*")) { }  // Compiles regex each time
}

// ✅ Pre-compiled pattern
private static final Pattern PATTERN = Pattern.compile("pattern.*");
for (String line : lines) {
    if (PATTERN.matcher(line).matches()) { }
}

// ❌ N+1 in loops
for (User user : users) {
    List<Order> orders = orderRepo.findByUserId(user.getId());
}

// ✅ Batch fetch
Map<Long, List<Order>> ordersByUser = orderRepo.findByUserIds(userIds);

問題となる点：

• ループ内で文字列を連結している
• ループ内で正規表現をコンパイルしている
• N+1 クエリのパターン
• 再利用できるオブジェクトを高頻度のループ内で生成している
• プリミティブストリーム（IntStream、LongStream）を使用していない

9. テストのヒント

次のテストを提案します：

• null 入力
• 空のコレクション
• 境界値
• 例外ケース
• 並行アクセス（該当する場合）

---

重要度のガイドライン

重要度	基準
重大	セキュリティ脆弱性、データ損失のリスク、本番環境でのクラッシュ
高	バグの可能性が高い、重大なパフォーマンス問題、API 契約への違反
中	コードの問題、保守性の問題、ベストプラクティスの不足
低	スタイル、軽微な最適化、提案

トークンの最適化

• 変更された行に注目します（git diff を使用します）
• 明らかな問題を繰り返し指摘せず、類似する指摘はまとめます
• コード全体を引用せず、行番号を示します
• 自動生成ファイルやテスト用フィクスチャは対象外にします

クイックリファレンスカード

カテゴリ	主な確認項目
Null 安全性	メソッド呼び出しの連鎖、Optional の誤用、null の返却
例外	空の catch、広すぎる catch、スタックトレースの消失
コレクション	反復処理中の変更、ストリームとループの使い分け
並行処理	共有される可変状態、確認してから実行する処理
慣用表現	equals/hashCode のペア、toString、ビルダー
リソース	try-with-resources、接続リーク
API	Boolean 型のパラメーター、null の処理、検証
パフォーマンス	文字列の連結、ループ内の正規表現、N+1