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

Singleton class extending a parent class to utilise shared functionality

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

Problem

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 getSingletonOb

Solution

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.

Context

StackExchange Code Review Q#6431, answer score: 2

Revisions (0)

No revisions yet.