4
\$\begingroup\$

Good Morning

This is a builder design pattern .. the builder problem is duplication so

I want to know from an expert with explanation which is better on quality on this code:

(1)

public class Animal {

    private String animalName;
    private int animalAge;

    public static class Builder {
        private Animal animal;

        public Builder() {
            animal = new Animal();
        }

        public Builder getName(String animalName) {
            animal.animalName = animalName;
            return this;
        }

        public Builder getAge(int animalAge) {
            animal.animalAge = animalAge;
            return this;
        }

        public Animal build(){
            animal.build(this);
            return animal;
        }
    }

    public Animal(){ }

    public void build(Builder builder) {
        this.animalName = builder.animal.animalName;
        this.animalAge = builder.animal.animalAge;
    }

    public String getAnimalName() {
        return animalName;
    }

    public int getAnimalAge() {
        return animalAge;
    } 
}

(2)

public class Animal {

    private String animalName;
    private int animalAge;

    public static class Builder{

        private String animalName;
        private int animalAge;

        public Builder getName(String animalName){
            this.animalName = animalName;
            return this;
        }

        public Builder getAge(int animalAge){
            this.animalAge = animalAge;
            return this;
        }


        public Animal build(){
            return new Animal(this);
        }

    }

    public Animal(Builder builder) {
        this.animalName = builder.animalName;
        this.animalAge = builder.animalAge;
    }

    public String getAnimalName() {
        return animalName;
    }

    public int getAnimalAge() {
        return animalAge;
    }   
}

in first model i pass animal object and create an instance and then the builder fill the data and build the animal again .

in second model i duplicate all the animal instance variables

which is true ? and which not broke builder rules ?

| improve this question | | | | |
\$\endgroup\$
2
\$\begingroup\$

From @pepijno 's answer:

What I've seen the most when it comes to builder patterns is that the object (in your case Animal) will get constructed in the build method of the builder. It is preferred to create instances of objects with all the data that the object should have, eg not creating invalid states of an object.

That is true. However his following code contradicts this.

public Animal(){ }

    public String getAnimalName() {
        return animalName;
    }

    public int getAnimalAge() {
        return animalAge;
    } 

So now I am free to create animal of age 0 and empty name? I know OP's code did not check for this. But if there were any checks like that (and they probably should), you are now free to create Animal with invalid state.

The way you should interpret the above quote is like this:

public class Animal
{
    private String animalName;
    private int animalAge;

    public Animal(String name, int age){ 
      if (name.length() == 0) {
         throw new Exception('Name cannot be empty');
      }
      if (age < 0) {
         throw new Exception('Age cannot be negative');
      }
      animalName = name;
      animalAge = age;
    }

    public String getAnimalName() {
        return animalName;
    }

    public int getAnimalAge() {
        return animalAge;
    } 
}

class AnimalBuilder
{
    private String animalName;
    private int animalAge;

    public AnimalBuilder(){}

    public AnimalBuilder setAnimalName(String name) {
        animalName = name;
        return this;
    }

    public AnimalBuilder setAnimalAge(int age) {
        animalAge = age;
        return this;
    }   

    public Animal build() {
      return new Animal(animalName, animalAge);
    }
}

Now, notice a few things.

AnimalBuilder is now ouside Animal class. This removes the circular dependency of the two classes. Builder knows the target Animal class, but the Animal is unaware of its building steps. At the same time, the Animal can be constructed without a builder and it cannot end up in an invalid state. This also explains why passing the builder to the animal constructor is bad, it would again introduce the circular coupling of the two classes (and also reminds service locator anti pattern). Same reason why in pepijno's answer, putting builder() method on the Animal class, is bad.

There might be more specific exceptions to use, but I don't know Java.

Also builder's methods should really be called set*, not get*, nor with* (as in pepijno's answer). The "withers" are usualy implemented in an immutable way:

class MyClass
{
  private String myName;
  private String other;

  public MyClass(String name, String x) {myName = name; other = x;}

  public MyClass withName(String name)
  {
    return new MyClass(name, other);
  }
}

As for the approach i noticed in comments https://codereview.stackexchange.com/a/127509/221293

Yes this is possible, but an interface should be created with only the getters and implemented by the Animal class to prevent consumers of constructed Animals from modifying it.

interface IAnimal
{
  public String getAnimalName();
  public int getAnimalAge();
}

public class Animal : IAnimal
{
    private String animalName;
    private int animalAge;

    public Animal(String name, int age){ 
      setAnimaName(name);
      setAnimalAge(age);
    }

    public String getAnimalName() {
        return animalName;
    }

    public void setAnimalName(String name) {
        if (name.length() == 0) {
           throw new Exception('Name cannot be empty');
        }
        animaName = name;
    }

    public int getAnimalAge() {
        return animalAge;
    } 

    public void setAnimalAge(int age) {
        if (age < 0) {
           throw new Exception('Age cannot be negative');
        }
        animalAge = age;
    }
}

And the builder should be declared to return that interface and not the class.

class AnimalBuilder
{
   ...
   public IAnimal build() {...}
   ...
}

But the Animal class should remain only constructible into a valid state, but the builder requires it to have default constructor (which restricts the usage to objects with default constructors, ie. where empty name is allowed).

And setting any property of the class after construction, must not corrupt the object. This to prevent bugs if someone decides to create the Animal directly, not using a builder, thus knowing its setters.

And this is why the approach from the mentioned SO post has restricted usage. It is also half the way between circular coupling and decoupling the two. Because although the Animal still does not know AnimalBuilder, it provides setters in expectation of existance of a builder that could use those setters (or it expects consumer to modify it, but that would be a completly different scenario than in your (OP) question).

It is also unsuitable when there are validations that depend on multiple properties (ie. if name is dog, his age not only must be non-negative, it cannot be more then 20 - ignore the nonsense of the example :D), because setting the two separately with distinct setters may lead to different behaviour depending on the order in which the setters are called.

The approach I proposed (high) above is generic and always applicable. The amount of repeating is about the same, IMO.

| improve this answer | | | | |
\$\endgroup\$
2
\$\begingroup\$

What I've seen the most when it comes to builder patterns is that the object (in your case Animal) will get constructed in the build method of the builder. It is preferred to create instances of objects with all the data that the object should have, eg not creating invalid states of an object. This prevents you from having illegal state in your program which might produce bugs. By constructing your Animal as soon as you create a builder instance you have to make your Animal mutable which is not always what you want. For example, in some cases a class might have only final fields which can only be assigned via constructor.

In your case, constructing the Animal in the build method it would look something like this:

public class Animal {

    private String animalName;
    private int animalAge;

    public static class AnimalBuilder {
        private String animalName;
        private int animalAge;

        public AnimalBuilder() {
        }

        public Builder withName(String animalName) {
            this.animalName = animalName;
            return this;
        }

        public Builder withAge(int animalAge) {
            this.animalAge = animalAge;
            return this;
        }

        public Animal build(){
            Animal animal = new Animal();
            animal.animalName = this.animalName;
            animal.animalAge = this.animalAge;
            return animal;
        }
    }

    public Animal(){ }

    public String getAnimalName() {
        return animalName;
    }

    public int getAnimalAge() {
        return animalAge;
    } 
}

You can then use it

Animal animal = new AnimalBuilder()
    .withName("Terry")
    .withAge(5)
    .build();

You can also add a static method builder() to the Animal class which returns a new builder:

public class Animal {
    ...

    public static AnimalBuilder builder() {
        return new AnimalBuilder();
    }
}

Animal animal = Animal.builder()
    .withName("Terry")
    .withAge(5)
    .build();

Does this mean a lot of duplication? Unfortunately, yes, builder patterns need a lot of duplicate code. You can also look at Lombok's Builder annotation which will generate all the builder boilerplate code for you. In that case it will just be

@Builder
public class Animal {

    private String animalName;
    private int animalAge;

    public Animal(){ }

    public String getAnimalName() {
        return animalName;
    }

    public int getAnimalAge() {
        return animalAge;
    } 
}
```
| improve this answer | | | | |
\$\endgroup\$
  • \$\begingroup\$ @pepjino thanks for your answer but my question didn't answered yet .. why passing Animal object as instance on Builder class is wrong ? \$\endgroup\$ – Ahmed Mamdouh Apr 3 at 9:50
  • \$\begingroup\$ @AhmedMamdouh I've edited my post, does this help answering your question? \$\endgroup\$ – pepijno Apr 3 at 10:01
  • \$\begingroup\$ @pepjino Thank you very much for your interest and help but i can't get it yet i'm sorry look i found a post and he use the object instance .. is his answer wrong ? can you explain .. and i'm apologize for wasting your time :)) codereview.stackexchange.com/a/127509/221293 << i mean in his case it's better to use object ? \$\endgroup\$ – Ahmed Mamdouh Apr 3 at 10:07
  • \$\begingroup\$ @AhmedMamdouh To be honest, that is the first time I've seen someone recommending create a builder like that. I prefer to create the object instance in the final step of the builder with the reasons I mentioned in my answer. It might be possible that both methods are used in different applications and that it will be a matter of personal preference. I'm afraid I cannot give a more clear answer, perhaps someone else will be better suited to give an answer regarding this matter. \$\endgroup\$ – pepijno Apr 3 at 10:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.