Singleton class extending a parent class to utilise shared functionality

Question

I have a singleton class which extends from an abstract java class. Two singleton classes extend from ItemImageThreadManager, the reason for this is to use shared scheduling functionality. A thread is created and passed into the runThread method of ItemImageThreadManager.

Is this good design?

Abstract Parent Class

public abstract class ItemImageThreadManager {
 private Timer timer = new Timer();
 public void runThread(final Thread runThread){
 try { 
 scheduleTimer(runThread);
 }
 catch(IllegalStateException ise){
 /**
 * When the timer is cancelled (when screen closed) and the images are 
 * removed from memory(in-app resync) and this screen is opened again, 
 * an IllegalStateException will be thrown since the images will need 
 * to be re-downloaded
 * 
 * Solution - create a new timer.
 */
 timer = new Timer();
 scheduleTimer(runThread);
 }
 }
 /**
 * Starts a new thread. This thread is run in the order is has been added.
 * Since timer runs in it's own thread, this gaurantees that just one thread 
 * will be started at one time....
 * @param thread
 */
 private void scheduleTimer(final Thread thread){
 timer.schedule(new TimerTask() {
 public void run() {
 thread.start();
 try { 
 thread.join();
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 }
 }, 0);
 }
 public void cancelTimer(){
 timer.cancel();
 }
}

First Extended class

public class ContestantImageThreadManager extends ItemImageThreadManager{
 private static ContestantImageThreadManager ref;
 private ContestantImageThreadManager(){
 super();
 }
 public static ContestantImageThreadManager getSingletonObject()
 {
 if (ref == null){
 ref = new ContestantImageThreadManager();
 }
 return ref;
 }
}

Second Extended class

public class JudgeItemImageThreadManager extends ItemImageThreadManager{
 private static JudgeItemImageThreadManager ref;
 private JudgeItemImageThreadManager(){
 }
 public static JudgeItemImageThreadManager getSingletonObject()
 {
 if (ref == null){
 ref = new JudgeItemImageThreadManager();
 }
 return ref;
 }
}

• Shahzeb: > Is this good design ? No . 1. Why do you need to two classes with same name ? It would not even compile in same package. 2. Why do you need Singleton ? I am not as anti-singleton as community in general is these days but certainly there is not obvious reason for this to be a singleton. You need to give us more background on problem domain .
• aishwarya: i actually don't see so much of a problem with this assuming you are going to extend this for your domain specific usage. the 2 singletons don't need to be singletons at the moment and runThread method could just have been static. But yes, if you are planning to do more in these classes, I think its fine. make your getSingleton methods synchronized though please.
• blue-sky: I added a differeing singleton class to question, I did'nt realise I copied and pasted the same singleton twice. I take your point about the singleton class though, it's possible I dont need it. Why should my getSingleton methods be synchronized ?
• blue-sky: This was very useful to descibe why a sington need to be synchronized - taskinoor.wordpress.com/2011/04/18/singleton_multithreaded
• Gyan: If they are not synchronized then it is possible for two threads to call getSingletonObject() at the same time and result in two instances of your singleton.

Answer 1

As others mentioned the getSingletonObject should be synchronized. There is a chapter in Effective Java, Second Edition about singletons (Item 3: Enforce the singleton property with a private constructor or an enum type). It's worth to read. Anyway, try to avoid them, they makes testing really hard.

You use the Timer as a single-threaded executor. In that case consider using Executors.newSingleThreadExecutor. Furthermore, are you sure that you need to pass Thread at all? Wouldn't Runnable be enough? Executors can run Runnables without any wrapper class and Thread.join call.

Instead of catching IllegalStateException in the runThread method you should check whether the timer is canceled. You can store this state in a boolean flag. Don't forget to synchronize the variable access. (See Effective Java, Item 57: Use exceptions only for exceptional conditions)

ExecutorService.submit returns Future which has a cancel method. Maybe it's better for you requirements (instead of Timer.cancel()).

If you're not familiar with concurrent code check Java Concurrency in Practice.