Reusing objects using a generic object pool After reading a lot of documents, I wrote this object pooling code. Can anyone help me to improve this code? I am lagging in validating the object, confirming whether I can reuse it or not.

/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */
package iaccount.ui;

import java.util.Enumeration;
import java.util.Hashtable;

/**
 *
 * @author system016
 */
public class ObjectPool&lt;T&gt; {

    private long expirationTime;
    private Hashtable locked, unlocked;

    ObjectPool() {
        expirationTime = 30000; // 30 seconds
        locked = new Hashtable();
        unlocked = new Hashtable();
    }
//   abstract 

    public Object create(Class&lt;T&gt; clazz) throws InstantiationException, IllegalAccessException {
        Object obj = clazz.newInstance();
        unlocked.put(clazz.newInstance(), expirationTime);

        return obj;

    }

    ;
//   abstract boolean validate( Object o );
//   abstract void expire( Object o );
    synchronized Object checkOut(Class&lt;T&gt; clazz) {
        long now = System.currentTimeMillis();
        Object o = null;

        if (unlocked.size() &gt; 0) {
            Enumeration e = unlocked.keys();
            while (e.hasMoreElements()) {
                o = e.nextElement();
                if ((clazz.isAssignableFrom(o.getClass()))) {

//                }
                    if ((now - ((Long) unlocked.get(o)).longValue())
                            &gt; expirationTime) {
                        // object has expired
                        unlocked.remove(o);
//                        expire(o);
                        o = null;

                    } else {
//                        if (validate(o)) {
                            unlocked.remove(o);
                            locked.put(o, new Long(now));
                            return (o);
//                        } else {
//                            // object failed validation
//                            unlocked.remove(o);
////                            expire(o);
//                            o = null;
//                        }
                    }
                }
            }
        }
        return o;
    }
//    public boolean validate(Object o){return true;};
}
//   synchronized void checkIn( Object o ){...}
The choices of classes and other design patterns that you have made indicate to me that the books/tutorials you have been reading are pretty old. There are a number of things in your code which are almost ten years out of date in the Java 'world'.

Also, you are only showing us half of your implementation, you have not included the checkIn() code.

While I can't review the whole system without the checkIn() code, there is a lot to comment on without that still.

General

It is obvious that you are using an IDE to help you write your code because you have some code-template things like:


/*
 * To change this template, choose Tools | Templates
 * and open the template in the editor.
 */



Why is that still there? Why have you not fixed it, or removed it?

Now, your tool should also then be pointing out other a bunch of other issues that you are obviously ignoring.... why?


You have a semicolon out of place:


    ;
//   abstract boolean validate( Object o );
//   abstract void expire( Object o );




Generics

Your class is defined as: public class ObjectPool&lt;T&gt; { but you do not take full advantage of generics.

Your constructor should take the variable Class&lt;T&gt; clazz instead of the create(Class&lt;T&gt; clazz) method, which should take nothing.

The constructor should save away the clazz as a private final Class&lt;T&gt; clazz;;

Then, your create() method should be fixed in 2 ways:


it should return generic type T
it should do some validation.
it should not have any input parameters.


To save having the complicated exceptions on the create method I recommend adding the first value to the pool in the constructor, and making the constructor throw the complicated exceptions.... then the create() method will be simpler to use ....

Note the following in these examples:


the instance variables should be private final (note the final).
Use the clazz instance to cast the value.


Example code:

    private final long expirationTime;
    private final Hashtable locked, unlocked;
    private final Class&lt;T&gt; clazz;

    ObjectPool(Class&lt;T&gt; clazz) throws InstantiationException, IllegalAccessException {
        expirationTime = 30000; // 30 seconds
        locked = new Hashtable();
        unlocked = new Hashtable();
        this.clazz = clazz;
        // complicated constructors with thrown exceptions....
        T o = clazz.cast(clazz.newInstance());
        locked.put(o, new Long(now));
    }

    public T create()  {
        try {
            T obj = clazz.cast(clazz.newInstance());
            unlocked.put(clazz.newInstance(), expirationTime);

            return obj;
        } catch (InstantiationException ie) {
            throw new IllegalStateException("Even though were were able to do it in the constructor, " 
                 + "we were not able to create a new instance of " + clazz.getName(), ie);
        } catch (IllegalAccessException iae) {
            throw new IllegalStateException("Even though were were able to do it in the constructor, " 
                 + "we were not able to access the newInstance() " + clazz.getName(), ie);
        }

    }


Your checkOut() method should have the same generic patterns as above.

Now, the two Hashtables... they should be Generified (your IDE is giving a lot of warnings, right?)

 private Hashtable&lt;T, Long&gt; locked, unlocked;


Locking and Synchronization

Your two methods are synchronized differently. The 'checkOut()method is synchronized, but thecreate` method is not.

Also, you are using the (old, by the way) class Hashtable which is internally synchronized.

You should, for simplicity at the moment, just use synchronized methods:

public synchronized T create() {
    ...
}

public synchronized T checkOut() {
    ...
}


and your Hashtable instances should both just be the newer, faster, and better:

private final Map&lt;T, Long&gt; locked, unlocked;


Note, I would normally recommend the Map should be a HashMap, but, read the next section...

Class Choices and bugs

Your class choice of Hashtable is wrong, it should be HashMap. This will lead to Iterator instead of Enumeration as well.

Also, you are making big assumptions about Class&lt;T&gt; since you use it as the key to the table/map. This is a problem because the keys need to be immutable. I would recommend using an java.util.IdentityHashMap instead.

Conclusion...

There are a lot of things which can be done differently. The code has a lot of work to do still