現場で見かけたJavaアンチパターン5選 — 「動くけど良くない」コードの直し方
はじめに
エンジニア3年目になって色んな現場のコードを読むようになると、「動いてはいるけど…これ大丈夫？」というコードに出会います。
新人の頃の自分も同じようなコードを書いていたので、責められないんですが、「なぜそれが良くないか」を知っているかどうかでその後の伸びが変わります。
この記事では、現場でよく見かけるJavaのアンチパターン5つと、その直し方をセットで紹介します。
全部「これだけは絶対ダメ」というルールではありません。「なぜ良くないか」を理解した上で、状況によって判断できるようになるのがゴールです。
アンチパターン1: getter/setterで何でも公開
よくあるコード
public class User {
private String name;
private int age;
private List<Order> orders;
public String getName() { return name; }
public void setName(String name) { this.name = name; }
public int getAge() { return age; }
public void setAge(int age) { this.age = age; }
public List<Order> getOrders() { return orders; }
public void setOrders(List<Order> orders) { this.orders = orders; }
}
IDEで自動生成された「全フィールドにgetter/setterがある」クラス。一見綺麗ですが、これは データクラス（貧血モデル） と呼ばれるアンチパターンの典型例。
何が良くないか
- どこからでも
user.setAge(-5)のような不正な値が代入できる - ロジックがクラスの外（サービス層）に散らばる
- 「Userは何ができるのか」が読めない
直し方
setterを廃止して、意図のあるメソッドに置き換える。
public class User {
private final String name;
private int age;
private final List<Order> orders = new ArrayList<>();
public User(String name, int age) {
this.name = Objects.requireNonNull(name);
if (age < 0) throw new IllegalArgumentException("age must be >= 0");
this.age = age;
}
public String getName() { return name; }
public int getAge() { return age; }
public void haveBirthday() { // 意図のあるメソッド
this.age++;
}
public void placeOrder(Order order) { // 意図のあるメソッド
this.orders.add(order);
}
public List<Order> getOrders() {
return List.copyOf(orders); // 防御的コピー
}
}
「setName/setAge/setOrders」じゃなく「haveBirthday/placeOrder」という名前にすると、Userが「何ができるか」が一目で分かります。
アンチパターン2: 巨大なif/elseチェーン
よくあるコード
public int calculateDiscount(String memberType) {
if (memberType.equals("BRONZE")) {
return 5;
} else if (memberType.equals("SILVER")) {
return 10;
} else if (memberType.equals("GOLD")) {
return 15;
} else if (memberType.equals("PLATINUM")) {
return 20;
} else {
return 0;
}
}
何が良くないか
- 会員種別が追加されるたびに
ifが増えていく -
memberTypeを文字列で渡しているので typo に気づけない（"GOLDD"でも コンパイルエラーにならない） - 同じ判定が他の場所にも散らばる
直し方
Enumに振る舞いを持たせる。
public enum MemberType {
BRONZE(5),
SILVER(10),
GOLD(15),
PLATINUM(20);
private final int discountRate;
MemberType(int discountRate) {
this.discountRate = discountRate;
}
public int getDiscountRate() {
return discountRate;
}
}
// 呼び出し側
int discount = memberType.getDiscountRate();
呼び出し側が1行に。新しい会員種別が増えても Enum に1行追加するだけ。Java 14以降ならSwitch式と組み合わせても綺麗です。
アンチパターン3: nullを返すメソッド
よくあるコード
public User findUser(String id) {
User user = userRepository.findById(id);
if (user == null) {
return null; // 見つからない時はnull
}
return user;
}
// 呼び出し側
User user = service.findUser("123");
if (user != null) { // nullチェック必須
System.out.println(user.getName());
}
何が良くないか
- 呼び出し側が nullチェックを忘れたらNullPointerException
- 「nullを返す可能性がある」ことがシグネチャから読めない
- nullチェックがコード中に散らばる
直し方
Optionalを使う。
public Optional<User> findUser(String id) {
return Optional.ofNullable(userRepository.findById(id));
}
// 呼び出し側
service.findUser("123")
.ifPresent(user -> System.out.println(user.getName()));
// または
String name = service.findUser("123")
.map(User::getName)
.orElse("名無し");
シグネチャに「ない場合がある」と書いてあるので、呼び出し側がnullチェック漏れに気づきやすくなります。
※ Optionalはフィールド型や引数型としては使わない。あくまで戻り値専用。
アンチパターン4: 例外を握り潰すcatch
よくあるコード
try {
doSomething();
} catch (Exception e) {
// 何もしない
}
または
try {
doSomething();
} catch (Exception e) {
e.printStackTrace(); // ログ出力だけして握り潰す
}
何が良くないか
- 本番でエラーが起きても気づけない
- 障害調査の時に何が起きたか追えない
- 「ここで例外が起きるはずがない」という思い込みのまま放置される
実際これで「数ヶ月前から壊れてたのに誰も気づかなかった」みたいなインシデントは現場あるあるです。
直し方
最低限ロガーで出力する、もしくは再スローする。
private static final Logger log = LoggerFactory.getLogger(MyService.class);
try {
doSomething();
} catch (IOException e) {
log.error("ファイル読み込みに失敗: path={}", path, e);
throw new ServiceException("読み込み失敗", e); // 適切な型でラップ
}
ポイント:
- 何が起きたか文脈付きでログ
- 必要なら適切な例外型に変換して再スロー
Exceptionで全部受けるのは最後の砦だけ
アンチパターン5: staticだらけのユーティリティ依存
よくあるコード
public class OrderService {
public Order create(OrderRequest req) {
// staticメソッド依存
String id = IdGenerator.generate();
LocalDateTime now = TimeUtils.now();
Order order = new Order(id, req.getItems(), now);
DbUtils.save(order);
return order;
}
}
「とりあえずstatic」で書かれたユーティリティクラス。便利ですが、これも罠があります。
何が良くないか
-
テストが書きづらい（
TimeUtils.now()を固定値にしたい時に詰む） - 依存関係がコードを読まないと分からない（コンストラクタに出てこない）
- モックがしづらく、Mockitoの static モック機能を引っ張り出す羽目になる
直し方
依存を「インスタンスとして注入する」。
public class OrderService {
private final IdGenerator idGenerator;
private final Clock clock;
private final OrderRepository repository;
public OrderService(IdGenerator idGenerator, Clock clock, OrderRepository repository) {
this.idGenerator = idGenerator;
this.clock = clock;
this.repository = repository;
}
public Order create(OrderRequest req) {
String id = idGenerator.generate();
LocalDateTime now = LocalDateTime.now(clock);
Order order = new Order(id, req.getItems(), now);
repository.save(order);
return order;
}
}
これでテストでは Clock.fixed(...) を渡せば時刻を固定できるし、IdGenerator もモックできる。Spring を使っているなら DI で自動的に解決してくれます。
※ 副作用のない純粋関数（
Math.max,String.formatなど）は static のままで OK。区別の基準は「外部状態に依存するか」。
まとめ
| アンチパターン | 直し方 |
|---|---|
| 何でもgetter/setter | 意図のあるメソッド名 + setter廃止 |
| 巨大if/elseチェーン | Enumに振る舞いを持たせる |
| nullを返すメソッド | Optionalを戻り値に |
| 例外を握り潰すcatch | ログ出力 + 必要なら再スロー |
| staticだらけのユーティリティ | コンストラクタ注入で依存を明示 |
おわりに
これらは全部「動く」コードです。コンパイルは通るし、テストも書けば通ります。
でも 「他の人が読んで安心できるか」「半年後の自分が直しやすいか」 で考えると、上の書き方の方が圧倒的に楽です。
新人の頃の自分は「動けばOK」だったんですが、複数の現場を見るうちに「動くだけのコードと、読みやすいコードは別物」と分かってきました。
1つでも「自分のコードでよく書いてるな…」と思うものがあったら、次のPRから直してみてください。レビューが圧倒的に通りやすくなります。