patternjavaMinor
ThreadPoolExecutor singleton
Viewed 0 times
singletonthreadpoolexecutorstackoverflow
Problem
I'm trying to build a
And I'm using it like this throughout the app:
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?
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:
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
-
Overly long names:
-
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
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
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.