After some interesting comments to my previous write-up I decided to make a follow-up post with some additional details.
Here is the code from Effective Java:
public class Person {
private final Date birthDate;
//...
private static final Date BOOM_START;
private static final Date BOOM_END;
static {
Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone("GMT"));
gmtCal.set(1946, Calendar.JANUARY, 1, 0, 0, 0);
BOOM_START = gmtCal.getTime();
gmtCal.set(1965, Calendar.JANUARY, 1, 0, 0, 0);
BOOM_END = gmtCal.getTime();
}
public boolean isBabyBoomer() {
return birthDate.compareTo(BOOM_START) >= 0 &&
birthDate.compareTo(BOOM_END) < 0;
}
}
and here's how I would have changed it:
public class BabyBoom {
private static final BabyBoom boom = new BabyBoom();
private Date start = null;
private Date end = null;
private BabyBoom() {}
public static BabyBoom getInstance() {
if (boom.start == null || boom.end == null) {
Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone("GMT"));
gmtCal.set(1946, Calendar.JANUARY, 1, 0, 0, 0);
boom.start = gmtCal.getTime();
gmtCal.set(1965, Calendar.JANUARY, 1, 0, 0, 0);
boom.end = gmtCal.getTime();
}
return boom;
}
public boolean contains(Date birthDate) {
return birthDate.compareTo(start) >= 0 && birthDate.compareTo(end) < 0;
}
}
public class Person {
private final Date birthDate;
//...
public boolean isBabyBoomer() {
BabyBoom boom = BabyBoom.getInstance();
return boom.contains(birthDate);
}
}
I didn't synchronize getInstance, because initializing the dates twice does no harm, so it's not worth the price of synchronization. However I did check that both fields are initialized before returning the object in getInstance.
Posts
2 comments:
I think in this specific case what you've done *is* safe (in the worst case where they're never published to other threads, 'start' and 'end' effectively act as thread-local).
But it's a fairly convoluted way of initialising those variables when you can just declare them final and initialise them in the constructor of BabyBoom. This way, you're piggybacking on the synchronization that the JVM needs to do every time a class is referred to in interpreted code (and which is probably optimised away in compiled code). Proving the code is correct is then a no-brainer. And whether you think the cost is high or not, you have no choice over classloading synchronization, so you may as well use it to your advantage.
you have no choice over classloading synchronization, so you may as well use it to your advantage
Hi Neil, in a general case - it can make my code simpler, but a chain of such initialization can cause snowball effect in the entire system. Let's imagine a contrived case where a system has no clock, and loading Calendar class gets stuck/blown, because it in turn statically initializes itself calling system clock... Should we be risking our application start-up because of that? This was the point of the first post.
In this particular example, since the only purpose of BabyBoom is related to Calendar, and we trust good ol' Calendar not to pull any tricks, your suggestion may be valid and it still improves original Effective Java example. We could also make BabyBoom a static inner class of Person, if it's not used anywhere else, and we'd still enjoy the benefit of it getting loaded only when isBabyBoomer is first called. But no static initializing of Calendar in Person.class, please :-)
Post a Comment