code-quality

コード品質レビューのスキル

クリーンコードの原則、API 設計、Java のベストプラクティスを組み合わせた体系的なコードレビューです。

使用するタイミング

• 「このコードをレビューして」／「コードレビュー」／「この PR を確認して」
• 「リファクタリング」／「このコードを整理して」／「読みやすさを改善して」
• 「API をレビューして」／「エンドポイントを確認して」／「REST レビュー」
• PR のマージ前、または API 変更のリリース前

レビュー戦略

1. 簡易スキャン - 意図を理解し、対象範囲を特定します
2. チェックリストの確認 - 以下の関連カテゴリを適用します
3. 要約 - 重大度順（Critical → Minor → Good）に指摘事項を列挙します

---

クリーンコードの原則

DRY - 繰り返しを避ける

違反例:

// ❌ Duplicated validation logic
public void createUser(UserRequest req) {
    if (req.getEmail() == null || !req.getEmail().contains("@")) {
        throw new ValidationException("Invalid email");
    }
}

public void updateUser(UserRequest req) {
    if (req.getEmail() == null || !req.getEmail().contains("@")) {
        throw new ValidationException("Invalid email");
    }
}

修正例:

// ✅ Single source of truth
public class EmailValidator {
    public void validate(String email) {
        if (email == null || !email.contains("@")) {
            throw new ValidationException("Invalid email");
        }
    }
}

KISS - シンプルに保つ

違反例:

// ❌ Over-engineered
public interface UserFactory {
    User createUser();
}
public class ConcreteUserFactory implements UserFactory {
    public User createUser() { return new User(); }
}

修正例:

// ✅ Simple
public User createUser() { return new User(); }

YAGNI - 必要になるまで作らない

違反例:

// ❌ Premature abstraction
public class ConfigurableUserServiceFactoryProvider { }

修正例:

// ✅ Implement when actually needed
public class UserService { }

---

API 契約のレビュー

HTTP メソッドの意味

メソッド	用途	冪等性	安全性
GET	リソースの取得	はい	はい
POST	新しいリソースの作成	いいえ	いいえ
PUT	リソース全体の置き換え	はい	いいえ
PATCH	部分更新	いいえ*	いいえ
DELETE	リソースの削除	はい	いいえ

よくある間違い:

// ❌ POST for retrieval
@PostMapping("/users/search")
public List<User> search(@RequestBody SearchCriteria criteria) { }

// ✅ GET with query params
@GetMapping("/users")
public List<User> search(@RequestParam String name) { }

// ❌ GET for state change
@GetMapping("/users/{id}/activate")
public void activate(@PathVariable Long id) { }

// ✅ POST/PATCH for state change
@PostMapping("/users/{id}/activate")
public ResponseEntity<Void> activate(@PathVariable Long id) { }

API のバージョン管理

// ✅ URL path versioning (recommended)
@RestController
@RequestMapping("/api/v1/users")
public class UserControllerV1 { }

// ❌ No versioning
@RequestMapping("/users")  // Breaking changes affect all clients

レスポンスのステータスコード

コード	用途	例
200 OK	GET／PUT／PATCH の成功	リソースが見つかった
201 Created	POST の成功	新しいリソースが作成された
204 No Content	DELETE の成功	リソースが削除された
400 Bad Request	バリデーションの失敗	入力が無効
404 Not Found	リソースが存在しない	ユーザーが見つからない
409 Conflict	状態の競合	メールアドレスが重複している
500 Server Error	予期しないエラー	データベースが停止している

DTO とエンティティの公開

// ❌ Exposing JPA entity
@GetMapping("/{id}")
public User getUser(@PathVariable Long id) {
    return userRepository.findById(id).get();  // Exposes internals, N+1 risk
}

// ✅ Use DTO
@GetMapping("/{id}")
public UserResponse getUser(@PathVariable Long id) {
    return userService.findById(id);  // Returns DTO
}

---

Java コードレビューのチェックリスト

Null 安全性

確認項目:

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

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

// ✅ Safe with early return
if (user.getName() == null) return "";
return user.getName().toUpperCase();

要注意事項:

• null チェックなしの連続呼び出し
• isPresent() を呼び出さずに Optional.get() を使用
• Optional や空のコレクションではなく null を返す
• 公開 API に @Nullable／@NonNull がない

例外処理

確認項目:

// ❌ Swallowing exceptions
try {
    process();
} catch (Exception e) { }  // Silent failure

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

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

要注意事項:

• 空の catch ブロック
• Exception または Throwable を捕捉している（範囲が広すぎる）
• 例外をログに記録していない
• 元の原因を指定せずに新しい例外を作成している

リソース管理

確認項目:

// ❌ Resource leak
FileInputStream fis = new FileInputStream(file);
String content = read(fis);
fis.close();  // Won't execute if read() throws

// ✅ Try-with-resources
try (FileInputStream fis = new FileInputStream(file)) {
    return read(fis);
}  // Auto-closed

トランザクション境界

確認項目:

// ❌ Missing transaction
public void createUser(UserRequest request) {
    User user = new User();
    userRepository.save(user);
    roleRepository.save(new Role(user));  // Two separate transactions
}

// ✅ Proper transaction
@Transactional
public void createUser(UserRequest request) {
    User user = new User();
    userRepository.save(user);
    roleRepository.save(new Role(user));  // Single atomic transaction
}

命名規則

良い例:

// ✅ Clear intent
public List<User> findActiveUsersByRole(String role) { }
public boolean isEmailValid(String email) { }
public void activateUser(Long userId) { }

悪い例:

// ❌ Unclear
public List<User> get(String s) { }
public boolean check(String str) { }
public void doStuff(Long id) { }

パフォーマンス

確認項目:

// ❌ N+1 query problem
List<User> users = userRepository.findAll();
for (User user : users) {
    List<Order> orders = orderRepository.findByUserId(user.getId());  // N queries
}

// ✅ Join fetch
@Query("SELECT u FROM User u LEFT JOIN FETCH u.orders")
List<User> findAllWithOrders();

// ❌ Loading all data
List<User> allUsers = userRepository.findAll();  // Could be millions

// ✅ Pagination
Page<User> users = userRepository.findAll(PageRequest.of(0, 20));

---

レビュー出力形式

## Code Review: [Component/Feature Name]

### Critical Issues
- **Null safety violation** (UserService.java:42) - `user.getName().toUpperCase()` can NPE. Use Optional or null check.
- **Resource leak** (FileHandler.java:15) - FileInputStream not closed. Use try-with-resources.

### Important Improvements
- **API design** - POST used for idempotent update (UserController.java:28). Use PUT instead.
- **Transaction missing** - Multi-step operation needs @Transactional (OrderService.java:56).
- **N+1 query** - Loop fetches orders individually (line 89). Use JOIN FETCH.

### Code Smells
- **Long method** - extractUserData() is 80 lines. Consider extracting sub-methods.
- **Magic number** - Use named constant instead of `86400` (line 123).
- **Inconsistent naming** - Mix of camelCase and snake_case in variables.

### Good Practices Observed
- ✅ Constructor injection used throughout
- ✅ DTOs properly separate from entities
- ✅ Comprehensive validation on all endpoints
- ✅ Good test coverage (87%)

---

クイックリファレンス：要注意項目

カテゴリ	危険信号
Null 安全性	連続呼び出し、Optional.get()、null の返却
例外	空の catch、広範な catch、スタックトレースの喪失
リソース	手動の close()、try-with-resources の欠如
API 設計	誤った HTTP メソッド、バージョン管理なし、エンティティの公開
トランザクション	@Transactional のない複数ステップの書き込み
パフォーマンス	N+1 クエリ、全データの読み込み、インデックスの欠如
クリーンコード	コードの重複、マジックナンバー、不明瞭な名前
Lombok 不使用	@Data、@Getter、@Setter、@Builder、@Slf4j、@RequiredArgsConstructor の使用 — 明示的なコードを使用してください

---

重大度レベル

• Critical - セキュリティ、データ損失、クラッシュのリスク → マージ前に必ず修正してください
• Important - パフォーマンス、保守性、正確性 → 修正することを推奨します
• Code Smell - スタイル、複雑さ、軽微な問題 → 対応は任意です
• Good - 良いプラクティスを促すための肯定的なフィードバック