6
\$\begingroup\$

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) {
        this.population = population;
    }

    public static void setEarth(Earth earth) {
        Earth.earth = earth;
    }
}

Client.java

package design.patterns;

public class Client {

    public static void main(String args[]){

        Earth earth = Earth.getInstance();

        //Invoke Earth's methods from the instance created on line 7
        System.out.printf("Earth is over %1$,.0f Days old. \n", earth.getAgeInYears());

        //Invoke Earth's methods from the Singleton directly
        System.out.println("Earth's orbit takes " + Earth.getInstance().getdaysForFullOrbit() + " days to complete.");

        //Invoke Earth's methods both ways
        System.out.println("Earth sits on a " + earth.getDegreesOfAxisTilt() + " degree tilt.");
        System.out.printf("As of 1/20/2014, there are %1$,.0f people on Earth!", + Earth.getInstance().getPopulation());
    }
}
\$\endgroup\$
5
\$\begingroup\$

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:

  1. It is not a singleton

    Earth real = Earth.getInstance();
    Earth.setEarth(null);
    Earth alternate = Earth.getInstance();
    if (real != alternate) {
       System.out.println("Oops...");
    }
    
  2. 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!

\$\endgroup\$
  • \$\begingroup\$ Wow, epic fail demonstrated by use of Eclipse's "Generate Getters and Setters" I didn't catch that it made that setEarth method! As far as the Synchronization, I was thinking that since the getInstance method is synchronized, that no 2 threads could access the object, but they clearly can since an object refrence could simply be passed to some concurrent operations. I guess there is no other way than synchronizing all of the getter and setters. Will update code sample in a sec. thanks or your review! \$\endgroup\$ – Shijima Jan 21 '14 at 3:42
  • \$\begingroup\$ @Shijima: A better way would be to avoid having all those setters in the first place. The getters aren't too terrible, i guess...aside from demoting objects to "value holders"....but seriously, should people be able to arbitrarily change the properties of Earth? :P Once you lose the setters, the need for synchronization of anything other than getInstance pretty much goes away. \$\endgroup\$ – cHao Jan 21 '14 at 9:07
3
\$\begingroup\$

According to the book Effective Java "a single-element enum type is the best way to implement a singleton". Example:

package example;

public enum Earth {
    INSTANCE;

    public double getAgeInYears() {
        return 4500000000L;
    }

    public float getdaysForFullOrbit() {
        return 365.26f;
    }

    public float getDegreesOfAxisTilt() {
        return 23.4f;
    }

    public double getPopulation() {
        return 7046000000L;
    }
}

Makes Client look like this:

package example;

public class Client {

    public static void main(String args[]){

        Earth earth = Earth.INSTANCE;

        //Invoke Earth's methods from the instance created on line 7
        System.out.printf("Earth is over %1$,.0f Days old. \n", earth.getAgeInYears());

        //Invoke Earth's methods from the Singleton directly
        System.out.println("Earth's orbit takes " + Earth.INSTANCE.getdaysForFullOrbit() + " days to complete.");

        //Invoke Earth's methods both ways
        System.out.println("Earth sits on a " + earth.getDegreesOfAxisTilt() + " degree tilt.");
        System.out.printf("As of 1/20/2014, there are %1$,.0f people on Earth!", + Earth.INSTANCE.getPopulation());
    }
}
\$\endgroup\$
  • \$\begingroup\$ While singleton enums are really easy to create and solve the readResolve issue when serializing the singleton for free, it is not always ideal to use enum - especially when dealing with application container where this class might get loaded by custom classloaders. As enums are singletons per se they are hardly collectable by the garbage collector and therefore may produce memory leaks as the whole classloader (and the classes it loaded get uncollectable -> OOM: PermGen). \$\endgroup\$ – Roman Vottner Jan 22 '14 at 22:04
  • \$\begingroup\$ I therefore try to use WeakSingletons instead, which get eligible for GC when no strong reference is pointing at them (not possible with enums sadly) which further has the limit of losing state (like f.e. a counter) if no class is referencing it and the GC collects "trash". This however can be bypassed by letting an object, which got loaded by the same classloader which further acts as manager and therefore exists as long as those classes are needed, hold a strong reference to this singleton(s). A bit more of work, but better memory management. \$\endgroup\$ – Roman Vottner Jan 22 '14 at 22:08

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.