current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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());
    }
}
share|improve this question
    
Post rolled back. Please don't update your original code after receiving an answer; that will invalidate them. You may add the updated code below the original, or you may post a follow-up question if you'd like further review. –  Jamal♦ Jan 21 at 3:55

2 Answers 2

up vote 4 down vote

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!

share|improve this answer
    
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! –  Shijima Jan 21 at 3:42
    
@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. –  cHao Jan 21 at 9:07
up vote 3 down vote

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());
    }
}
share|improve this answer
    
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). –  Roman Vottner Jan 22 at 22:04
    
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. –  Roman Vottner Jan 22 at 22:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.