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

ThreadPoolExecutor singleton

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

Problem

I'm trying to build a ThreadPoolExecutor singleton to use it across my app. I'm very new to this concept and after going through Google docs I have created this:

public class MyThreadPoolExecutor {

    // Sets the amount of time an idle thread will wait for a task before terminating
    private static final int KEEP_ALIVE_TIME = 1;

    // Sets the Time Unit to seconds
    private static final TimeUnit KEEP_ALIVE_TIME_UNIT;

    // Sets the initial threadpool size to 8
    private static final int CORE_POOL_SIZE = 8;

    // Sets the maximum threadpool size to 8
    private static final int MAXIMUM_POOL_SIZE = 8;    // A static block that sets class fields

    private final BlockingQueue MyBlockingQueue;

    // A managed pool of background download threads
    private final ThreadPoolExecutor MyPoolExecutor;

    // A single instance of PhotoManager, used to implement the singleton pattern
    private static MyThreadPoolExecutor MyThreadPoolInstance = null;

    static {

        // The time unit for "keep alive" is in seconds
        KEEP_ALIVE_TIME_UNIT = TimeUnit.SECONDS;

        // Creates a single static instance of PhotoManager
        MyThreadPoolInstance = new MyThreadPoolExecutor();
    }

    private MyThreadPoolExecutor() {
        MyBlockingQueue = new LinkedBlockingQueue();

        MyPoolExecutor = new ThreadPoolExecutor(CORE_POOL_SIZE, MAXIMUM_POOL_SIZE,
                KEEP_ALIVE_TIME, KEEP_ALIVE_TIME_UNIT, MyBlockingQueue);
    }

    public static MyThreadPoolExecutor getInstance() {

        return MyThreadPoolInstance;
    }

    public void queueJob(Runnable r){

        MyPoolExecutor.execute(r);
    }

}


And I'm using it like this throughout the app:

MyThreadPoolExecutor threadPool = MyThreadPoolExecutor.getInstance();
            threadPool.queueJob(new MyRunnable());


It looks fine to me, but I can't find good reference of how to do it. Is the above correct, or is it the right way to implement such a thing?

Solution

The good:

Well I was actually struggling a little to find places in your code, where you have made no mistakes whatsoever (when seen as whole), but actually there's a lot of good things in your code:

  • Correct use of the right data structures



  • Extraction of "magic numbers" to properly named constants



  • Correct implementation of the singleton pattern



  • Clean and simple initialization



But wherever there's light there's also shadows.

The bad:

-
You are overcommenting. Comments that merely restate what happens in the line right below them are useless noise. Comments like that don't explain anything new.

You can actually assume, that people reading your code can program, there's no necessity to spell things out for them. After all they are intelligent ;)

Instead your comments should shed light on complicated algorithms, that are hard to follow, even when written cleanly. Also important to comment is the why of code that's "different" to the standard. Further reading

-
You're violating naming conventions. Fields, no matter whether they are static or not, should be named in camelCase instead of pascal case. This was the reason Vladimir thought your code didn't compile.

-
Overly long names: MyThreadPoolInstance could just be instance. This also applies to MyPoolExecutor and MyBlockingQueue, which should be executor and queue respectively.

-
And while we're at the queue. It's never used. If you need it use it, if you don't need it: get rid of it.

The ugly:

There's two kinda ugly things about your code. The first being that you are misusing a static initializer block where it's completely unnecessary. Instead you could just do the eager initialization directly. Your executor and KEEP_ALIVE_TIME_UNIT do not depend on any ordering.

The second thing is that you're using the Singleton pattern. The Singleton pattern is often a misinformed choice. The thought of using a Singleton often comes from needing "a central ... for ..."

That is in 90% of the cases possible to solve without resorting to the Singleton pattern. This is especially true in your case. Often singletons get misused as a "Utility class". Enforcing Singleton use is problematic, because it can easily become a bottleneck in your application if done wrongly.

Additionally in your case (as in most) it's completely unnecessary. Instead you should strive to make your code just use the built-ins of java, as demonstrated in Vladimir's answer

Context

StackExchange Code Review Q#84367, answer score: 7

Revisions (0)

No revisions yet.