Reusing objects using a Generic Object Pool

@rolfl made a good and complete review. I would add only one thing that I think is very important.

Don't leave commented code.

//                        } else {
//                            // object failed validation
//                            unlocked.remove(o);
////                            expire(o);
//                            o = null;
//                        }

Why is it commented ? Is it still good, is there a bug ? Can I safely remove it ? Commented code tend to stick in place forever, because no one want to remove it. Normally, your code will be under the control of a versioning software. If you need a previous version of the code, you will update a previous version of the file. This will help having only the code needed and remove noise when people read the code.

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<T> { but you do not take full advantage of generics.

Your constructor should take the variable Class<T> clazz instead of the create(Class<T> clazz) method, which should take nothing.

The constructor should save away the clazz as a private final Class<T> clazz;;

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

1. it should return generic type T
2. it should do some validation.
3. 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:

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

Example code:

    private final long expirationTime;
    private final Hashtable locked, unlocked;
    private final Class<T> clazz;

    ObjectPool(Class<T> 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<T, Long> 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<T, Long> 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<T> 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