Java | オブジェクト指向：実務でよくある設計ミス

Java Java
スポンサーリンク

実務でよくある「設計ミス」とは何か

実務でよく見る設計ミスは、「一発でシステムが壊れるような爆発」ではなく、
最初は普通に動くけれど、時間が経つほどコードが重く・遅く・触りたくなくなっていく、そんなタイプが多いです。

共通しているのは、
「オブジェクトに役割をちゃんと持たせていない」
「短期的なラクを取って、長期的なツラさを溜め込んでいる」
というところです。

ここから、実務で本当によく見るものに絞って、例と一緒に解説していきます。

巨大な God クラス・なんでも屋 Service

どういうミスか

UserService や OrderService が、
ユーザ登録も、ログインも、メール送信も、通知も、全部持っているパターンです。

public class UserService {

    public void register(User user) { ... }

    public void login(String email, String password) { ... }

    public void sendWelcomeMail(User user) { ... }

    public void resetPassword(User user) { ... }

    public void exportUsersToCsv(OutputStream out) { ... }

    // 他にも20メソッドくらい…
}
Java

最初は「どうせ処理は少ないし、1クラスでいいか」で始まりがちですが、
機能が増えるたびにここへ足されていき、
気づくと 2000 行級の怪物になります。

なぜ問題か（重要）

一つのクラスに責務が集中すると、

どこに何が書いてあるか分かりにくい
テストの単位が大きくなり、モックだらけのテストになる
ちょっとした変更でも他の機能に影響しやすい
新入りが触るのが怖くなる

という状態になります。

「このメソッドを直したら、他のどこに影響する？」が読めなくなっていくのが、一番のダメージです。

どう直すかのイメージ

役割ごとに分割します。

ユーザ登録専用の UserRegistrationService
ログイン専用の LoginService
メール送信は UserMailService

というように、「一言で説明できる責務」単位でクラスを分けていきます。

public class UserRegistrationService {
    public void register(User user) { ... }
}

public class LoginService {
    public void login(String email, String password) { ... }
}

public class UserMailService {
    public void sendWelcomeMail(User user) { ... }
}
Java

クラス名を見ただけで「何をしてくれる人か」分かるようになれば、かなり健全です。

DTO と Entity の境界崩壊

どういうミスか

画面や API の入出力用に作った DTO を、そのままドメインロジックでも使い倒してしまうパターンです。

public class UserDto {
    public String name;
    public String email;
    public int point;
}

// Controller
userService.addPoint(userDto);

// Service
public void addPoint(UserDto userDto) {
    userDto.point += 100;
}
Java

最初は「DTO も Entity も似たようなもんでしょ」で済ませたくなりますが、これをやると
「外向きの形」と「中で守るべきルール」がごちゃ混ぜになっていきます。

なぜ問題か（重要）

DTO は「データの入れ物」であって、「業務ルールを守る人」ではありません。

DTO をロジックの主役にしてしまうと、

どこでも public フィールドを書き換えられる
バリデーションや不変条件を守る場所がバラバラになる
API 仕様の変更がドメインロジックに直撃する

といったことが起きます。

「名前は必須」「ポイントはマイナス不可」といったルールは、
User エンティティが守るべきであり、DTO が守るべきではありません。

どう直すかのイメージ

境界で DTO と Entity を変換して、
ドメイン内では Entity だけを扱うようにします。

public class User {
    private String name;
    private String email;
    private int point;

    public void addPoint(int added) {
        if (added < 0) throw new IllegalArgumentException();
        point += added;
    }
}
Java
public class UserController {

    public void addPoint(UserDto dto) {
        User user = userRepository.find(dto.id);
        user.addPoint(dto.addedPoint);
        userRepository.save(user);
    }
}
Java

「DTO は境界だけ」「中では Entity と値オブジェクト」
と意識するだけで、設計の安定感が段違いになります。

public フィールドと setter だらけのクラス

どういうミスか

とにかく全部 public + getter/setter にしてしまうパターンです。

public class User {
    public String name;
    public String email;
    public int point;
}
Java

あるいは、

public class User {
    private String name;
    private String email;
    private int point;

    public String getName() { return name; }
    public void setName(String name) { this.name = name; }

    // 以下、全フィールドに getter/setter
}
Java

「カプセル化してます」と言いつつ、
実質 public フィールドと大差ない状態になっています。

なぜ問題か（重要）

どこからでも setPoint(-1000) のようなことができてしまうので、

名前は null
メールは空文字
ポイントはマイナス

といった「ありえない状態」のオブジェクトが簡単に生まれます。

オブジェクト指向で一番大事なのは、
「オブジェクト自身が自分の一貫性を守ること」です。

全部 setter で丸裸にすると、それを完全に諦めることになります。

どう直すかのイメージ

変更したい操作を「意味のあるメソッド」に変えます。

public class User {

    private String name;
    private String email;
    private int point;

    public User(String name, String email, int point) {
        changeName(name);
        changeEmail(email);
        addPoint(point);
    }

    public void changeName(String newName) { ... 必須チェック ... }

    public void changeEmail(String newEmail) { ... 形式チェック ... }

    public void addPoint(int added) { ... マイナス禁止、上限チェック ... }

    public String name() { return name; }
    public String email() { return email; }
    public int point() { return point; }
}
Java

「そのクラスにとって意味のある操作」だけを公開して、
それ以外の勝手な書き換えはさせないようにする。
これが、不変条件を守るための設計の芯です。

static ユーティリティだらけ・new 地獄でテスト不能

どういうミスか

とりあえず全部 static メソッドにしたり、
クラスの中で依存オブジェクトを直接 new しまくるパターンです。

public class UserService {

    public void register(User user) {
        // DB保存
        Db.save(user);

        // メール送信
        MailUtil.sendWelcome(user.getEmail());
    }
}
Java
public class SomeService {

    public void doSomething() {
        ExternalApi api = new ExternalApi("https://...");
        api.call();
    }
}
Java

static 直呼びや new 直書きは、最初はラクですが、
「差し替え」「テスト」がほぼ不可能になります。

なぜ問題か（重要）

テストで何が困るかを想像すると分かりやすいです。

MailUtil.sendWelcome() が本当にメールを送ってしまう
ExternalApi が本物の外部サービスに接続してしまう

「ここだけスタブにしたい」「呼ばれたかだけ確認したい」というときに、
差し込む余地がありません。

結果として、

ユニットテストを諦めて手動テストに頼る
バグが出ても再現や切り分けがしんどい

という方向に進みがちです。

どう直すかのイメージ

依存はインターフェースにして、外から注入します。

public interface MailSender {
    void sendWelcomeMail(String email);
}

public class SmtpMailSender implements MailSender { ... }
Java
public class UserService {

    private final MailSender mailSender;

    public UserService(MailSender mailSender) {
        this.mailSender = mailSender;
    }

    public void register(User user) {
        // DB保存
        userRepository.save(user);

        // メール送信
        mailSender.sendWelcomeMail(user.email());
    }
}
Java

テスト時にはテスト用の実装を渡せばよく、

MailSender dummy = new DummyMailSender();
UserService service = new UserService(dummy);
Java

のようにして、「モックしやすい設計」に変えられます。

レイヤーの混在：Controller で全部やる・Repository が賢すぎる

どういうミスか

Controller の中にビジネスロジックも SQL も全部書くパターン。

public class UserController {

    public void register(HttpServletRequest req, HttpServletResponse res) {
        String name = req.getParameter("name");
        String email = req.getParameter("email");

        Connection con = dataSource.getConnection();
        PreparedStatement ps = con.prepareStatement("INSERT ...");
        // …
        // ついでにポイントやステータスのロジックもここに
    }
}
Java

あるいは Repository がビジネスルールを持ち始めるパターン。

public class UserRepository {

    public void addPoint(long userId, int point) {
        // ビジネスルールを加味した更新SQLをここで組む…
    }
}
Java

レイヤー（Web、アプリケーション、ドメイン、永続化）がごちゃごちゃになります。

なぜ問題か

責務が混ざると、

同じロジックを別の画面やバッチで再利用しづらい
テストしたいときに Web や DB ごと立ち上げないといけない
「どこを読めばルールが分かるか」が不明瞭になる

といった問題が出てきます。

Controller は「HTTP の入り口」だけ、
Repository は「永続化の詳細」だけ、
ドメインやサービス層が「ルールの中心」、
と役割を分けておいたほうが、長期的には楽です。

どう直すかのイメージ

例えばユーザ登録なら、

Controller
→ リクエスト DTO を受け取って Service に渡すだけ

Service
→ DTO から Entity を作り、ドメインロジックを実行し、Repository を呼ぶ

Repository
→ Entity を DB に保存・読み出しするだけ

に分割します。

public class UserController {

    private final UserRegisterService service;

    public void register(UserRegisterRequestDto request) {
        service.register(request);
    }
}
Java
public class UserRegisterService {

    private final UserRepository userRepository;

    public void register(UserRegisterRequestDto request) {
        User user = new User(request.name, request.email);
        userRepository.save(user);
    }
}
Java

「どの層が何をやるか」を整理し直すのも、立派なリファクタリングです。

名前でごまかしてしまう設計ミス

どういうミスか

クラス名・メソッド名が曖昧で、本当の責務が隠れてしまうパターン。

CommonUtil
DataManager
MainService
SubService
doProcess()
execute()

など、実務で腐るほど見かけます。

なぜ問題か

名前が曖昧だと、

何をやるクラスか分からない
別の似たような名前のクラスが増えていく
「とりあえずここに足してしまえ」が起きやすい

つまり、設計崩壊の入口になります。

どう直すかのイメージ

名前を変えようとすると、責務が炙り出されます。

DataManager → 「何のデータを管理してる？」 → UserRepository や OrderRepository に分割
MainService → 「メインって何？」 → MonthlyBillingService など具体名にする
doProcess() → 「何を処理してる？」 → sendInvoice() や calculateTotal() にする

名前を変えようとして単語に詰まるときは、
そのクラスやメソッドが色々やりすぎているサインです。
中身を分けるチャンスでもあります。

迷ったときに自分へ投げるといい質問

設計ミスは、「そのときは早く終わらせたい気持ち」とセットで出てきます。
だからこそ、書いている途中で一回立ち止まって、こんな問いを投げてみてください。

このクラスは、一言で言うと何をするクラスか
このメソッドは、本来どのクラスの責務なのか
この if / switch は、どこかのオブジェクトの振る舞いにできないか
このフィールドは、自由に書き換えられても本当に安全か
この設計のまま機能が3倍になっても、まだ耐えられそうか

スポンサーリンク
タイトルとURLをコピーしました