Spring BootのControllerでやってはいけないこと

新人SE

Controllerに全部書いたら動きました。これでもダメなんですか？

ポンコツSE

動くことと、現場で保守できることは別です。Controllerは入口に集中させ、業務判断やDB操作はServiceやRepositoryへ分けるのが基本です。

Spring Bootを学び始めると、最初に触ることが多いのがControllerです。URLを受けて画面やJSONを返せるので、つい何でもControllerに書きたくなります。

しかし現場では、Controllerが太くなるとレビューでかなり高い確率で指摘されます。理由は、Controllerが読みづらくなるだけでなく、テストしづらく、仕様変更にも弱くなるからです。

Controllerの役割は入口に集中すること

Controllerの基本的な役割は、HTTPリクエストを受け取り、必要な値を取り出し、Serviceを呼び出して、結果をレスポンスとして返すことです。

@RestController
@RequestMapping("/users")
public class UserController {
    private final UserService userService;

    public UserController(UserService userService) {
        this.userService = userService;
    }

    @GetMapping("/{id}")
    public UserResponse getUser(@PathVariable Long id) {
        return userService.findUser(id);
    }
}

このくらいであれば、Controllerを読んだ人は「どのURLで、どのServiceを呼び、何を返すのか」をすぐに理解できます。

やってはいけない1：業務ロジックをControllerに書く

新人がやりがちなのは、Controllerの中でステータス判定や金額計算、権限チェックなどの業務処理まで書いてしまうことです。

@PostMapping("/orders/{id}/cancel")
public ResponseEntity<?> cancel(@PathVariable Long id) {
    Order order = orderRepository.findById(id).orElseThrow();

    if ("SHIPPED".equals(order.getStatus())) {
        return ResponseEntity.badRequest().body("出荷済みのためキャンセルできません");
    }
    if ("CANCELED".equals(order.getStatus())) {
        return ResponseEntity.badRequest().body("すでにキャンセル済みです");
    }

    order.setStatus("CANCELED");
    orderRepository.save(order);
    return ResponseEntity.ok().build();
}

このコードは動くかもしれません。しかし、Controllerが注文キャンセルの業務ルール、DB取得、DB更新、レスポンス作成をすべて抱えています。

現場では、注文キャンセルの判断はControllerではなくServiceへ寄せる方が自然です。

@PostMapping("/orders/{id}/cancel")
public ResponseEntity<Void> cancel(@PathVariable Long id) {
    orderService.cancel(id);
    return ResponseEntity.ok().build();
}

Controllerから見ると、注文をキャンセルするというユースケースだけが見えるようになります。細かい業務判断はService側で扱います。

やってはいけない2：RepositoryをControllerから直接呼ぶ

検索だけならControllerからRepositoryを呼んでもよさそうに見えることがあります。しかし、現場ではControllerからRepositoryを直接呼ぶと、責務が混ざりやすくなります。

@GetMapping("/users/{id}")
public UserEntity detail(@PathVariable Long id) {
    return userRepository.findById(id).orElseThrow();
}

この書き方では、ControllerがDBアクセスとEntity返却を直接行っています。最初は短く見えますが、権限チェック、削除済み判定、表示用DTO変換が増えると一気に崩れます。

@GetMapping("/users/{id}")
public UserResponse detail(@PathVariable Long id) {
    return userService.findDetail(id);
}

Serviceを挟むことで、ControllerはHTTPの入口、Serviceは業務判断、RepositoryはDBアクセスという役割を保ちやすくなります。

やってはいけない3：Entityをそのまま返す

APIでEntityをそのまま返すのも、レビューでよく指摘されます。DB用の構造と画面やAPIで返したい構造は、必ずしも同じではないからです。

@GetMapping("/users/{id}")
public UserEntity getUser(@PathVariable Long id) {
    return userService.findEntity(id);
}

Entityには、画面に出したくない項目、内部管理用の項目、リレーション情報が含まれることがあります。そのまま返すと、不要な情報を外に出したり、JSON変換で予期しない問題が起きたりします。

public class UserResponse {
    private Long id;
    private String name;
    private String email;
}

画面やAPI用には、必要な項目だけを持つDTOを返す方が安全です。

やってはいけない4：長いif文をControllerに置く

入力値やステータスに応じてレスポンスを変える処理はあります。しかし、Controllerに長いif文が並ぶと、何の業務ルールなのか分かりにくくなります。

if (request.getAmount() == null) {
    return ResponseEntity.badRequest().body("金額は必須です");
}
if (request.getAmount().compareTo(BigDecimal.ZERO) <= 0) {
    return ResponseEntity.badRequest().body("金額は1円以上です");
}
if (!userService.exists(request.getUserId())) {
    return ResponseEntity.badRequest().body("ユーザーが存在しません");
}

単純な入力チェックならバリデーション機能を使い、業務ルールならServiceへ寄せます。Controllerは、どのユースケースを呼ぶかが分かる程度に留めると読みやすくなります。

現場レビューではどう指摘されるか

// レビューコメント例
// Controllerにキャンセル可否の業務判定とRepository呼び出しが入っています。
// Controllerはリクエスト受け取りとレスポンス返却に寄せて、
// キャンセル処理はOrderServiceへ移してください。

この指摘は、きれいごとの設計論ではありません。キャンセル処理が画面、API、バッチなど複数の入口から使われる可能性があるため、Controllerに閉じ込めない方が保守しやすいという話です。

Controllerに書いてよいもの

逆に、業務上の判断、DB更新、外部API連携、複雑なDTO変換は、Controllerから外へ出す候補です。

レビュー前のセルフチェック

この記事とあわせて読みたい

• Controller Service Repositoryの役割
• Spring BootのServiceに業務ロジックを書く理由
• Spring BootのDTO変換はどこでやるべきか

まとめ

Controllerは、Spring Bootアプリケーションの入口です。何でも書ける場所ではなく、リクエストを受け取り、Serviceを呼び、レスポンスを返す場所として使うと、現場でレビューされにくいコードになります。