I just finished studying immutable objects and their advantages so I thought I'd create one of my own. Here is my employee class that extends a person class. It is not mutable since I have getters to my mutable date objects.
package objects.objects;
import java.util.Date;
public class Person {
private final String name;
private final Date dateOfBirth;
Person(String name,Date dateOfBirth){
this.name=name;
this.dateOfBirth=dateOfBirth;
}
public String getName() {
return name;
}
public Date getDateOfBirth() {
return dateOfBirth;
}
}
Employee Class:
package objects.objects;
import java.util.Date;
public final class Employee extends Person{
private final String empolyeeID;
private final Designation designation;
private final double salary;
private final Date dateOfJoining;
public Employee(String Name,Date dateOfBirth,String employeeID,Designation designation,double salary,Date dateOfJoining)
{
super(Name,dateOfBirth);
this.empolyeeID=employeeID;
this.designation=designation;
this.salary=salary;
this.dateOfJoining=dateOfJoining;
}
public String getEmpolyeeID() {
return empolyeeID;
}
public Designation getDesignation() {
return designation;
}
public double getSalary() {
return salary;
}
public Date getDateOfJoining() {
return dateOfJoining;
}
}
The designation enum:
package objects.objects;
public enum Designation {
ASSOCIATE,SENIOR_ASSOCIATE,MANAGER,SENIOR_MANAGER,DIRECTOR
}
The Oracle Java documentation says:
Don't share references to the mutable objects. Never store references to external, mutable objects passed to the constructor; if necessary, create copies, and store references to the copies. Similarly, create copies of your internal mutable objects when necessary to avoid returning the originals in your methods
Furthermore, the Oracle doc says:
Don't allow subclasses to override methods. The simplest way to do this is to declare the class as final. A more sophisticated approach is to make the constructor private and construct instances in factory methods.
What if I want my class to be subclassed? What harm is there if I don't make my class final or have a private constructor?
Does it mean I shouldn't use getters or maybe pass a copy (a new Date object in the getter?) or is there some other approach that I am missing?
Also, could you point out apart from the date being mutable object references? Is there any other flaw in my code that violates the mutable policy or any other improvements as such?
8 Answers 8
All your fields are private and final, which is the first step to being immutable. This is good.
The Date class is not immutable, so, for example, you have a problem with the method:
public Date getDateOfBirth() { return dateOfBirth; }
because, someone could do:
employee.getDateOfBirth().setYear(1900);
and suddenly your employee has aged... a lot.
What the oracle documentation is saying, is that, if you have mutable content, you should return a copy of that data. This is often called a 'defensive copy':
public Date getDateOfBirth() {
return new Date(dateOfBirth.getTime());
}
Update: additionally, your method should be final so that no subclasses can change the behaviour of getDateOfBirth() (same is true for getName()). If the method is not final, then a subclass could possibly override the method, and do it in a way which make the name/dateOfBirth mutable.
Note, that the constructor for Date, in this instance, takes a long value. This leads on to a suggestion that the immutable class should store a long instead of a Date. The long is immutable anyway, if final.
public class Person {
private final String name;
private final long dateOfBirth;
.....
public Date getDateOfBirth() {
return new Date(dateOfBirth);
}
}
Apart from that, the immutability looks fine. Good, even.
The package name you have chosen though... is lacking: objects.objects. You could not name your package something better?
-
7
-
8\$\begingroup\$ You forgot to mention that the
Datemust be copied before it gets stored, too. Obviously, the switch tolongdoes the job. \$\endgroup\$maaartinus– maaartinus2014年10月14日 12:16:09 +00:00Commented Oct 14, 2014 at 12:16 -
1\$\begingroup\$ Absolutely, I was already thinking ahead to the long, so I assumed.... Copy-on-store too. \$\endgroup\$rolfl– rolfl2014年10月14日 12:19:02 +00:00Commented Oct 14, 2014 at 12:19
-
\$\begingroup\$ @rolfl The doc says : "Don't allow subclasses to override methods. The simplest way to do this is to declare the class as final. A more sophisticated approach is to make the constructor private and construct instances in factory methods"... What if i want my class to be subclassed? what harm is there if i dont make my class final or have a private constructor? I am unable to follow this line \$\endgroup\$Ishan Soni– Ishan Soni2014年10月14日 13:21:28 +00:00Commented Oct 14, 2014 at 13:21
-
1\$\begingroup\$ @IshanSoni - there are a lot of things that need to be considered when evaluating immutability. I went through your code and did not see many problems (though I missed some, and multiple/other answers are great that way). Answering "what makes a class immutable?" is different to answering "is this class immutable?". As for subclassing, a subclass could declare its own
dateOfBirth, override the existing method, and store it 'wrong'. yourgetName()andgetDateOfBirth()methods should be final. Will modify my answer. \$\endgroup\$rolfl– rolfl2014年10月14日 13:26:33 +00:00Commented Oct 14, 2014 at 13:26
In addition to protecting getDateOfBirth as @rolfl already explained,
it's equally important to make defensive copies in constructors too:
Person(String name, Date dateOfBirth) {
this.name = name;
this.dateOfBirth = new Date(dateOfBirth.getTime());
}
Otherwise, if you do simply this.dateOfBirth = dateOfBirth;, then somebody could write this code:
Person jack = new Person("Jack", date);
date.setYear(1900); // ouch!
Since mutable objects such as Date are so troublesome,
it's a lot safer to store the long value of dateOfBirth.getTime() instead of the Date itself.
That way you simply don't need to remember to use defensive copies,
your long value will always remain immutable, as opposed to Date objects.
If you want the safest possible immutable class, then you should definitely go with this option, and store the long values representing dates. The same goes for dateOfJoining in Employee as well.
This answer isn't about immutability since @rolfl has it covered.
You should put spaces between your = in your assignations, it is easier to spot the = so it makes it easier to see the assignation.
There is lots of white spaces between your getters from Employee, one should be enough otherwise the code takes alot of space for nothing.
You have a parameter that is PascalCased instead of camelCased, is it something java specific that makes it impossible to name it name? Otherwise, you should name it.. name!
As mentioned in different answers, Date is a mutable structure so you need to copy it every time you want to share it. My advice is not use at all, and use an immutable structure like Joda Data.
The type of salary is double, and thats wrong, you should be using BigDecimal for money and not doubles, and make sure you use the right constructor which is the one that takes a String as parameter
BigDecimal salary = new BigDecimal("502.34");
BigDecimal itself is immutable, so you don't need to copy it.
What if I want my class to be subclassed? What harm is there if I don't make my class final or have a private constructor?
The harm lies in that you have no way of enforcing that these subclass instances are themselves immutable. Depending on what properties of immutability you rely on, this can throw off otherwise working code.
Does it mean I shouldn't use getters or maybe pass a copy (a new Date object in the getter?) or is there some other approach that I am missing?
Immutability is not a matter of whether or not your class has getters; in particular, a class without setters does not make an immutable class. In its purest definition, immutability is the absense of mutability. An object is immutable if, and only if, there exists no way to mutate its state.
In Java, this implies what the documentation says:
- Don't share references to the mutable objects. Because that would mean that other code can change your internal state.
Don't allow subclasses to override methods. Because subclasses can change the behaviour and thus the observable state of an object. Consider:
public class ImmutablePoint { // mutable class; share no references! private final Point p; public ImmutablePoint(Point p) { // copy values, do not store reference this.p = new Point(p.x, p.y); } public double getX() { return p.getX(); } public double getY() { return y.getY(); } } // no longer immutable public class ExtensionPoint extends ImmutablePoint { private Point offset = new Point(); public ExtensionPoint(Point origin) { super(origin); } public void setOffset(int x, int y) { offset.x = x; offset.y = y; } public double getX() { return super.getX() + offset.getX(); } public double getY() { return super.getY() + offset.getY(); } } public class JitteryPoint extends ImmutablePoint { public JitteryPoint(Point origin) { super(origin); } // Here there be shenanigans public double getX() { return super.getX() + Math.random(); } public double getY() { return super.getY() + Math.random(); } }
I would like to mention another aspect which hasn't been mentioned by most others, and that's your constructor
public Employee(String, Date, String, Designation, double, Date)
The more arguments you add to a constructor, the harder it gets to use because the order of parameters isn't clear anymore. When you have multiple parameters of the same type, like in this case two Strings and two Dates, it becomes very easy to mix them up which causes a bug which is hard to spot when reading the code.
A good way to handle this issue for immutable objects is to make the constructor private and call it from a Builder class which is a public class declared inside the class it builds (it needs to be inside to be able to call a private constructor).
Employee.Builder builder = new Employee.Builder();
builder.setName(name);
builder.setDateOfBirth(dateOfJoining);
builder.setEmployeeID(id);
builder.setDesignation(designation);
builder.setSalary(salary);
builder.setDateOfJoining(birthdate);
Person p = builder.build();
The builder collects all the attributes you set in private variables. When you call build(), it calls the constructor of Employee with the attributes set before and returns the result. When one attribute isn't set yet, you can either use a reasonable default or throw an exception when no default makes sense.
By the way, do you see the bug I have in the above code? Would you be able to spot it if I had called the constructor directly?
When you have each setter of the Employee.Builder return this, you can also use a very elegant syntax known as "fluent interface":
Person p = new Employee.Builder().setName(name)
.setDateOfBirth(birthdate)
.setEmployeeID(id)
.setDesignation(designation)
.setSalary(salary)
.setDateOfJoining(dateOfJoining)
.build();
Something of a nit in light of the other very good answers, but you've consistently misspelled employee as empolyee.
Contrary to what some people say, there is nothing fundamentally wrong with having an inheritable immutable class provided that the inheritance contract dictates that if two or more instances are ever regarded as equivalent, they must always and forevermore be regarded as equivalent; further, the class must work correctly without complaint or visibly altered behavior if some or all references to an object are replaced with references to a object which is considered "equivalent". A class won't be able to prevent the definition of illegitimate derived classes that violate its inheritance contract if its methods aren't declared final, but since it won't be able to prevent the definition of illegitimate derived classes even if its methods are final, so making the methods final doesn't particularly matter. What does matter is that the inheritance contract specifies that all derived classes must be fully immutable not only in their inherited fields, but also in all observable characteristics, inherited and otherwise. If a derived class doesn't abide by that contract, code which uses the derived class might malfunction, but the fault will lie entirely with the derived class that violates the contract.
It is probably a good idea to make immutable types final if one doesn't want to specify exactly what will be required and may be expected of any derived types. There are, however, many situations where it may be helpful to have an abstract base class or interface which specifies that all legitimate derived classes or implementations will be immutable. For example, one might define a Matrix2d class or interface with members to get the dimensions or read the cell at any (row,column) coordinate. Although the most common implementation might use a 2d array as a backing store, there are many different ways that derived classes might store information. If Matrix2d were a final class which used an array as a backing store, then all objects that were usable as type Matrix2d would have to have a backing-store element for every cell even if 99% [or for that matter 100%] were blank. Making it a non-final class would make it possible to define derivatives like DiagonalMatrix2d which could use a much simpler backing store.
Employeeis final? \$\endgroup\$