Effective JavaにおけるBuilderパターンを用いた実装についての指摘に関して

Question

お世話になります。
掲題の件について意見をお聞かせ下さい。

先日、Effective JavaにおけるBuilderパターンを用いた実装を行ったところ、コードレビューにおいて、

「Builder自身をコンストラクタの引数に渡してしまうと、コンストラクタ内でもしbuild()を呼び出された場合に無限の再帰呼び出しとなってしまう為よろしくない。やるのであれば、Builderの内部でパラメタ用のクラスを組み立て、それを渡すような作りが良い。」

との指摘を受けました。

Effective JavaのBuilderの実装そのものに対する指摘になっている感じなのですが。
プラスでパラメタクラスを作るのも無駄な感じがあり、
外部に対して渡している訳でもなく、正直そこまで考慮する必要はないのでは？と感じたのですが、意見をお聞きしたく。

　
よろしくお願いします。

Comments

[swordone]

どういうコードなのか掲載してください。

[Zax_XK]

質問の意図が不明瞭で、伝わり辛く申し訳ありません。自身が書いたコードに対するというより、BuilderパターンでコンストラクタにBuilderを渡す事に対して、コンストラクタ内でbuild()が呼ばれた場合の考慮不足であるという指摘点に関して、そこまで考慮する必要があるかどうかという意見を参考までにお聞きしたかった次第です。Builderの書き方や構成はhttps://qiita.com/disc99/items/840cf9936687f97a482b等で紹介されているEffective Javaのパターンと同一のものです。

Answer 1

Effective JavaのBuilderの実装そのものに対する指摘になっている感じなのですが。

本に書いてあるから正解とは言えないですね。
指摘内容も妥当な懸念点だと思います。

外部に対して渡している訳でもなく、正直そこまで考慮する必要はないのでは？と感じたのですが、

build()を呼ぶことには、下記と同じことだと思います。

private static class A {
    public A() {
        new B();
    }
}
private static class B {
    public B() {
        new A();
    }
}

このようなコード書くことは防げないが、テストコード、レビュー等でカバーする必要がありますね。

プラスでパラメタクラスを作るのも無駄な感じがあり、

Effective JavaにおけるBuilderパターンは、私も見たことあって、よくないな思ったことあります。
具体的には、生成対象のオブジェクトと同じ属性を定義しているところです。
今回指摘された内容を踏まえて、下記のように書くのはどうかなと。

public class People {

    private String name;
    private Integer age;
    private String hobby;

    static class Builder {
        private People people = new People();

//        private String name;
//        private Integer age;
//        private String hobby;

        Builder(String name, Integer age) {
            people.age = age;
            people.name = name;
        }

        Builder hobby(String hobby) {
            people.hobby = hobby;
            return this;
        }

        People build() {
            if (people.name == null || people.age == null) {
                throw new NullPointerException();
            }

            return this.people;
        }
    }

//    private People(Builder builder) {
//        this.name = builder.name;
//        this.age = builder.age;
//        this.hobby = builder.hobby;
//    }

    String hello() {
        return "hello";
    }
}

Comments

[Zax_XK]

ご回答いただき有難うございます。

>本に書いてあるから正解とは言えないですね。
仰る通りです。
Effective Java自体はかなり有名な書籍ですが、だからといってそれをそのまま鵜呑みにするのは良くないなと今回のレビューを以て考えさせられた次第です。
特にネット上ではそのままの形でお勧めされていたりするので、特に初心者や中級者であればあまり深く考えずに使ってしまうなと思います。（まさに今回の自分ですね。。）

>build()を呼ぶことには、下記と同じことだと思います。
>このようなコード書くことは防げないが、テストコード、レビュー等でカバーする必要がありますね。
なるほど確かに、このように書くことは防ぎようがないし、等価と考えるとレビューやテストでフォローする必要があるなと思わされます。

>具体的には、生成対象のオブジェクトと同じ属性を定義しているところです。
その点は自分も感じました。
二重に同じプロパティを書く必要がある事で、変に無駄を増やしているような気がします。

具体的な改善例もご提示いただいて大変有難うございます。
貴重なご意見、参考とさせていただきます。