Problem
This is a builder pattern implementation to build immutable Person objects:
Person class
public final class Person {
// All are set to final to make Person Immutable
private final int id;
private final String name;
private final String surname;
private final boolean isOccupied;
// Constructor is private, so that only static
// PersonBuilder can initiate the Person class instance
private Person(PersonBuilder builder) {
this.id = builder.getId();
this.name = builder.getName();
this.surname = builder.getSurname();
this.isOccupied = builder.isOccupied();
}
public static class PersonBuilder {
// Member variables of PersonBuilder are temporary storage
// to create Immutable Person class instance
private int id;
private String name;
private String surname;
private boolean isOccupied;
PersonBuilder() { }
// The only method to initiate Person class
Person build() {
return new Person(this);
}
// Multiple Constructors for each member variable
public PersonBuilder id(int id) {
this.id = id;
return this;
}
public PersonBuilder name(String name) {
this.name = name;
return this;
}
public PersonBuilder surname(String surname) {
this.surname = surname;
return this;
}
public PersonBuilder setOccupied(boolean isOccupied) {
this.isOccupied = isOccupied;
return this;
}
// getters, these will be used in private constructor
// of Person class
public int getId() {
return id;
}
public String getName() {
return name;
}
public String getSurname() {
return surname;
}
public boolean isOccupied() {
return isOccupied;
}
}
// getters
public int getId() {
return id;
}
public String getName() {
return name;
}
public String getSurname() {
return surname;
}
public boolean isOccupied() {
return isOccupied;
}
@Override
public String toString() {
return String.format(
"Id:tt%dnName:tt%snSurname:t%snIsOccupied:t%sn"
, id, name, surname, isOccupied);
}
}
Person demo
public class PersonDemo {
public static void main(String[] args) {
// PersonBuilder instance
Person.PersonBuilder builder = new Person.PersonBuilder();
// builder builds the instance
builder.name("Levent").surname("Divilioglu").id(666).setOccupied(true);
// Immutable Person object is initiated with build() method
Person person = builder.build();
System.out.println(person);
}
}
Demo output
Id: 666 Name: Levent Surname: Divilioglu IsOccupied: true
I did write this implementation to learn the GoF builder pattern. Immutability is important for thread-safety, thus set the class and it’s members final. I also set the Person constructor private to guarantee that it is only instantiated with inner class method, and only once. On each build() method, a new unique Person instance will be created.
I’m aware that builder patter is used for classes that have a lot of many member variables but I did only used 4 member variables for the simplicity.
Solution
First of all, very good job.
Now I will point out a few things that I think can be improved on.
Code Duplication
You have copied over all the private fields from Person to the Builder, which is necessary when the fields are declared final. As coderodde pointed out, using a final declaration is not necessary here when the fields are private and there are no setters.
Now that the fields are no longer declared final, it is possible to hold an unfinished version of your Person object in your builder. Now all the fields from Person no longer need to be copied over.
This also prevents the need for the getter methods on your Builder and the weird constructor of Person that takes a Builder as a parameter. The Person class should not be initializing itself via the state of the Builder object. The Builder should be building the Person, hence the name of the pattern.
Excessive commenting
You use comments a lot, even at places where they aren’t needed and are generally just noisy. For example:
// getters
and
// getters, these will be used in private constructor
// of Person class`.
The first one is simply redundant, since all the methods below it are called get..., clearly indicating a getter function. The second one also provides little useful information. Note that generally, someone will read a code file from top to bottom, unless they might be looking for the details on a specific method. This would mean that they have already scrolled past your constructor and seen the getters being used there.
But there is a bigger point I want to make here with regards to the comments. In general, using comments to express what your code does, means that you have failed to express yourself in code, which can document itself when names are chosen carefully.
Coming back to the second comment I mentioned. You probably wrote that because you felt that you had to explain yourself for having public getters on your Builder class. When placing a comment like this, take a step back and ask yourself why you placed that comment and if your code would still seem logical without it. If your code seems weird without the comment, which I think public getters on a Builder class do, then you need to fix that issue instead of commenting about why you did it.
Now by moving the initialization of the Person to the Builder class where it belongs, as mentioned above, the Builder no longer needs these methods and the comment can be removed.
The same goes for the comment about the temporary member variables, which can now be removed as the Builder simply holds an instance of the Person class, which makes perfect sense on its own.
I went on a bit of a rant about comments, but I hope you can see what I mean.
Naming
Overall, job well done with the naming. A few remarks though:
- I’d rename
PersonBuildertoBuilder. It’s an inner class ofPersonso there really is no need for it.Person.Builderis perfectly fine. - I’d also rename the individual building methods for
id,name,surnameandisOccupiedtosetId,setName,setSurnameandsetOccupied, since they are all effectively setters.
Reworked Code
When incorporating these points into your code, it should look a little something like this.
public final class Person {
private int id;
private String name;
private String surname;
private boolean isOccupied;
private Person() {}
public static class Builder {
private Person personToBuild;
Builder() {
personToBuild = new Person();
}
Person build() {
Person builtPerson = personToBuild;
personToBuild = new Person();
return builtPerson;
}
public Builder setId(int id) {
this.personToBuild.id = id;
return this;
}
public Builder setName(String name) {
this.personToBuild.name = name;
return this;
}
public Builder setSurname(String surname) {
this.personToBuild.surname = surname;
return this;
}
public Builder setOccupied(boolean isOccupied) {
this.personToBuild.isOccupied = isOccupied;
return this;
}
}
public int getId() {
return id;
}
public String getName() {
return name;
}
public String getSurname() {
return surname;
}
public boolean isOccupied() {
return isOccupied;
}
@Override
public String toString() {
return String.format(
"Id:tt%dnName:tt%snSurname:t%snIsOccupied:t%sn"
, id, name, surname, isOccupied);
}
}
You can simplify your builder by removing the final specifiers from the fields of Person. Don’t worry, unless you add setters for the person type, it is effectively immutable.
toString() is not usually used as a method for printing human-readable output, but rather used as an aid in debugging.
I had this in mind:
public final class Person {
// The class has no setters, so it's effectively immutable. We need a
// possibility of setting to these fields so that we can construct them in
// the builder.
private int id;
private String name;
private String surname;
private boolean isOccupied;
// Hide the constructor from the user.
private Person() {}
public static final class Builder {
private Person person;
// Initialize state.
private Builder(final Person person) {
this.person = person;
}
// Start building a person.
public static Builder build() {
return new Builder(new Person());
}
public Builder id(final int id) {
this.person.id = id;
return this;
}
public Builder name(final String name) {
this.person.name = name;
return this;
}
public Builder surname(final String surname) {
this.person.surname = surname;
return this;
}
public Builder occupied(final boolean isOccupied) {
this.person.isOccupied = isOccupied;
return this;
}
// End construction and return a unique person object.
public Person get() {
final Person ret = new Person();
ret.id = person.id;
ret.name = person.name;
ret.surname = person.surname;
ret.isOccupied = person.isOccupied;
return ret;
}
}
// getters
public int getId() {
return id;
}
public String getName() {
return name;
}
public String getSurname() {
return surname;
}
public boolean isOccupied() {
return isOccupied;
}
@Override
public String toString() {
return String.format("[id: %d, name: %s, surname: %s, occupied: %b]",
id,
processNull(name),
processNull(surname),
isOccupied);
}
private static String processNull(final String str) {
return str == null ? "null" : """ + str + """"""