HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Simple Java Singleton

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
singletonsimplejava

Problem

Below is a Singleton that I designed as a piece of sample code for a website I'm working on for fun. The goal is to use the Singleton to hold an Earth object since Earth is unique and there is only 1 of them. Please let me know your thoughts on the implementation.

Singleton.java

```
package design.patterns;

/**
* This class is a Singleton, we can only have 1 instance of it no matter what!
*
* We mark the class final so that no other classes try to extend it. Some
* designs would want the Singleton to be extendable but we do not!
*/
public final class Earth {

// A static instance of an Earth object
private static Earth earth;

// Some earthly variables
private long ageInYears = 4500000000L;
private float daysForFullOrbit = 365.26f;
private float degreesOfAxisTilt = 23.4f;
private long population = 7046000000L;

// Prevent client from instantiating new Earth objects
private Earth() {
}

/**
* Global access point (no pun intended :)
*
* Synchronized so its thread safe.
*/
public static synchronized Earth getInstance() {

// "Lazy load" an Earth Object
if (earth == null) {
earth = new Earth();
}

return earth;
}

// Basic getters and setters
public double getAgeInYears() {
return ageInYears;
}

public void setAgeInYears(long ageInYears) {
this.ageInYears = ageInYears;
}

public float getdaysForFullOrbit() {
return daysForFullOrbit;
}

public void setdaysForFullOrbit(float daysForFullRotation) {
this.daysForFullOrbit = daysForFullRotation;
}

public float getDegreesOfAxisTilt() {
return degreesOfAxisTilt;
}

public void setDegreesOfAxisTilt(float degreesOfAxisTilt) {
this.degreesOfAxisTilt = degreesOfAxisTilt;
}

public double getPopulation() {
return population;
}

public void setPopulation(int population) {
th

Solution

The code you have is nicely formatted, and well documented, etc. ... but, as a singleton, it has a number of problems....

The two most glaring are:

-
It is not a singleton

Earth real = Earth.getInstance();
Earth.setEarth(null);
Earth alternate = Earth.getInstance();
if (real != alternate) {
   System.out.println("Oops...");
}


-
The synchronization.

You suggest in your comments that the getInstance() needs to be synchronized to avoid thread problems... but your other setter/getter methods are not synchronized.... as a result, threads all over the place can be getting stale, wrong, and otherwise incomplete populations, ages, etc.

As an example of a singleton 'best use case', this one has some problems... ;-)

But, that's sort of OK, since Earth has problems anyway!

Code Snippets

Earth real = Earth.getInstance();
Earth.setEarth(null);
Earth alternate = Earth.getInstance();
if (real != alternate) {
   System.out.println("Oops...");
}

Context

StackExchange Code Review Q#39710, answer score: 5

Revisions (0)

No revisions yet.