my very first pool

Thread view:

R - 15 Apr 2006 10:10 GMT

Hi All,

I'm new to Java, I wrote my very first pool - share your comments on my
Pool, please.
I'm not fully familiar with all Java specific constructions,
conventions and standards - that's why I'm asking for comments.

If I'm doing sth wrong pleas point it out, if sth could be done better
please suggest it also.

thanks in advance for any comments
best regards
R

Here it is:
// pool for DAOFactories objects

abstract class Pool {
    public static final int POOL_MAX_SIZE = 5;
    private static List<DAOFactory> pool;
    private static Logger log =
Logger.getLogger(Pool.class.getCanonicalName());

    static
    {
        log.setLevel(Level.ALL);
        log.finest("Initialization of pool");
        pool = new ArrayList<DAOFactory>(Pool.POOL_MAX_SIZE);
        for (int i = 0; i < Pool.POOL_MAX_SIZE; i++) {
            pool.add(new DAOFactory());
        }
    }

    public static DAOFactory getDAOFactory()
    {
        synchronized (pool) {
            if (false == pool.isEmpty()) {
                return pool.remove(pool.size() - 1);
            }
        }
        return null;
    }

    public static boolean resleaseDAOFactory(DAOFactory df)
    {
        synchronized (pool) {
            return pool.add(df);
        }
    }

    public static int getSize() {
        synchronized (pool) {
            return pool.size();
        }
    }
}

Reply to this Message

Oliver Wong - 15 Apr 2006 15:48 GMT

> Hi All,
>
[quoted text clipped - 9 lines]
> best regards
> R

Spell words out completely. Instead of writing "sth", write "something".
I'm assuming that's what you meant by "sth" anyway... maybe I was wrong.

> Here it is:
> // pool for DAOFactories objects
>
> abstract class Pool {

Do you intend for your Pool to be subclassed? I'd assume not, but the
"abstract" keyword tells me you want someone to subclass Pool. I'd remove
that keyword. If the intent is to make it impossible to instantiate Pool,
simply declare a private constructor.

> public static final int POOL_MAX_SIZE = 5;
> private static List<DAOFactory> pool;
> private static Logger log =
> Logger.getLogger(Pool.class.getCanonicalName());

It looks like your Pool class doesn't actually depend on the DAOFactory
class. Have you considered making the Pool generic, so that it can be used
to pool any kinds of classes? You'd probably have to make Pool
instantiatable though (either that, or use something like a static
Map<Class<T>,List<T>> to map from classes to the pools of those classes).

If you're gonna make Pool instantiable, might as well make POOL_MAX_SIZE
configurable as well.

> static
> {
[quoted text clipped - 10 lines]
> synchronized (pool) {
> if (false == pool.isEmpty()) {

Roedy will tell you not to compare equality against false, but instead
to write something like "!pool.isEmpty()"

> return pool.remove(pool.size() - 1);
> }
> }
> return null;
> }

Seeing as how you've got the synchronized keyword, I'm assuming you plan
to use this in multithreaded apps. Have you considered having this method
block until a DAOFactory is ready, rather than returning null, when the pool
is empty? Of course, you'll end up with deadlock if the same thread tries to
get more than POOL_MAX_SIZE instances. Maybe you can record which threads
are requesting what, and complain if one of them gets more than a certain
amount (e.g. 1)?

> public static boolean resleaseDAOFactory(DAOFactory df)
> {
> synchronized (pool) {
> return pool.add(df);
> }
> }

There's a spelling mistake in the above method name.

> public static int getSize() {
> synchronized (pool) {
> return pool.size();
> }
> }
> }

- Oliver

Reply to this Message

my very first pool

Free Magazines