5
\$\begingroup\$

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<Runnable> 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<Runnable>();

        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?

\$\endgroup\$
6
\$\begingroup\$

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:

  1. 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

  2. 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.

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

  4. 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

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I watched a movie in this answer :) \$\endgroup\$ – Prudhvi May 31 '15 at 1:28
5
\$\begingroup\$

As far as I see, you just need to submit task into thread pool with fixed threads amount. Much easier and safer to delegate it to java.util.concurrent.Executors helper class:

public static void main(String[] args) throws InterruptedException {
    // 8 threads in pool
    ExecutorService executorService = Executors.newFixedThreadPool(8);

    executorService.submit(new MyRunnable());

    // request shutdown executor when all work will be done
    executorService.shutdown();

    // when shutdown() has been called, executor actually does not stop and continue work
    // so you should specify termination timeout
    executorService.awaitTermination(2, TimeUnit.MINUTES);
}

Also you should assign values to variables. Currently that won't compile:

MyThreadPoolInstance = new MyThreadPoolExecutor();

fix:

MyThreadPoolInstance instance = new MyThreadPoolExecutor();
|improve this answer|||||
\$\endgroup\$
  • 2
    \$\begingroup\$ Note that The question uses non-conventional capitalization for field names, so MyThreadPoolInstance is a field name, not a class name. Logically, the OP's code is: private static MyThreadPoolExecutor MyThreadPoolInstance = new MyThreadPoolExecutor();, which does compile. \$\endgroup\$ – rolfl Apr 4 '15 at 11:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy