HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Object pool implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
poolobjectimplementation

Problem

Below is my implementation of a pool. It is based on a hash table and supports using strong, soft or weak references to store objects. At the start there is a builder class to configure and create instances, followed by a subclass of Pool which allows multiple reference types to be used. Towards the end there is an enum defining the reference types available and an implementation of the Node interface (used to store objects) for each of them.

The Nodes are stored and accessed the same way as in HashMap (minus the trees) and WeakHashMap, in a array of buckets which has a length equal to a power of two with the index being obtained by &ing the length of the array minus one and a modified (to account for only using the lower bits) version of the hash code submitted. Each Node has a reference to the next Node in the bucket, or null if it is the last.

It registers Nodes with a ReferenceQueue and attempts to clean out any that are in the queue when the methods that add or remove objects or check the size of the pool are called. Nodes whose values have been finalized are also removed during re-sizes and when encountered while retrieving objects.

I don't have any specific concerns about my code, I'm just looking for any ways that I can improve.

```
package common.collections;

import java.lang.ref.ReferenceQueue;
import java.lang.ref.SoftReference;
import java.lang.ref.WeakReference;
import java.util.Arrays;
import java.util.Objects;
import java.util.function.Predicate;
import java.util.function.Supplier;
import java.util.function.UnaryOperator;
import java.util.stream.Stream;

import common.utils.EnumUtils;
import common.utils.StreamUtils;

/** A pool based on a hash table which can store its elements in {@code Reference} objects.
*
* Null values are not accepted and will result in a {@code NullPointerException} if you
* try to add them.
*
This class is not thread safe. /
public class Pool {
private static final int MAX_CAPACITY = 1

Solution

/** Removes all objects from this pool. */
@SuppressWarnings("unchecked")
public void clear() {
    size = 0;
    int s = getNewSize();
    if (s == nodes.length) {
        Arrays.fill(nodes, null);
    } else {
        nodes = new Node[s];
    }
    while (queue.poll() != null);
}


The function is doing something else besides what the comment says (namely, running a while loop). What's the while loop for? Additionally, you should (this is a nitpick) write a comment why you use a @SuppressWarnings annotation there.

private T find(Predicate equals, int hash,
        UnaryOperator> replacement) {
    int index = hash & (nodes.length - 1);
    for (Node n = nodes[index], last = null; n != null; n = n.next()) {
        if (n.hash() != hash) {
            last = n;
            continue;
        }
        T t = n.get();
        if (t == null) {
            size--;
            if (last == null) {
                nodes[index] = n.next();
            } else {
                last.setNext(n.next());
            }
            continue;
        }
        if (equals.test(t)) {
            Node newn = replacement.apply(n);
            if (newn == null) {
                size--;
                if (last == null) {
                    nodes[index] = n.next();
                } else {
                    last.setNext(n.next());
                }
            } else if (newn != n) {
                newn.setNext(n.next());
                if (last == null) {
                    nodes[index] = newn;
                } else {
                    last.setNext(newn);
                }
            }
            resize();
            return t;
        }
        last = n;
    }
    resize();
    return null;
}


This function has a cyclomatic complexity of 10. That's a bit high, but that's okay. Nothing you can do about it. However, you should keep in mind that as your functions get more complex, your variable names should be more descriptive. What does newn mean? Is it a new Node? I don't see the keyword new though. Additionally, such complex methods could use commentary to explain just exactly what they do (this comment of mine might become irrelevant once you've made the variable names more descriptive).

More @SuppressWarnings show up - comment those with an explanation for a possible maintenance programmer.

My personal preferences (nitpicks):

I'd add a comment why you return this on the builder (I know why, it's for chaining, but others might not.).

I'd also rename the functions growFactor and shrinkFactor to setGrowFactor and setShrinkFactor. This because it seems easy to mistake them for "growByFactor" and "shrinkByFactor". The documentation shows that this is not the case however, which makes this my personal preference rather than a notable improvement.

I'd put blank lines between the end of a function and the start of a comment block documenting the next function. Doing so gives me the feeling that a function ends there and that I can process what I've read before continuing.

private void resize() {
    int s = getNewSize();
    if (s != nodes.length) {
        resize(s);
    }
}
private int getNewSize() {
    int s = nodes.length;
    while (size > s * growFactor && s  MIN_CAPACITY) {
        s >>= 1;
    }
    return s;
}
@SuppressWarnings("unchecked")
private void resize(int length) {


Personally, I prefer to keep functions with the same name together. This makes it easier to look them up. So in this case, I'd swap resize() with getNewSize().

Now we're really getting into nitpick-land.

/** Extension of {@link Pool#Add(T)} which allows the caller to specify the
     * reference type to be used to store the object. If a matching object is found
     * with a different reference type its type is changed to {@code referenceType} if
     * {@code changeCurrent} returns true. If {@code referenceType} is null the pool's
     * reference type is used instead. */


"If a matching object is found with a different reference type its type is changed to referenceType if changeCurrent returns true."

This sentence is missing a comma. Change to "(...)with a different reference type, its type".

"If referenceType is null the pool's reference type is used instead."

Also missing a comma. Change to "If `referenceType is null, the pool's (...)".

At this point I think we can agree that you've written a marvelous piece of code. It's well documented, makes full use of the language's features (loop labels! Last time I saw those is when I was reading the spec for fun!), and doesn't seem to have any obvious flaws. As you can see, none of the things I pointed out relate to the actual code, save for the naming of some functions. The rest is all related to the comments and order of your functions.

I'd suggest analyzing the code with various tools such as Sonar and whatever else is out there. I'd also suggest writing a set of unittests if you really wanted to polish it to perfection.

Code Snippets

/** Removes all objects from this pool. */
@SuppressWarnings("unchecked")
public void clear() {
    size = 0;
    int s = getNewSize();
    if (s == nodes.length) {
        Arrays.fill(nodes, null);
    } else {
        nodes = new Node[s];
    }
    while (queue.poll() != null);
}
private T find(Predicate<? super T> equals, int hash,
        UnaryOperator<Node<T>> replacement) {
    int index = hash & (nodes.length - 1);
    for (Node<T> n = nodes[index], last = null; n != null; n = n.next()) {
        if (n.hash() != hash) {
            last = n;
            continue;
        }
        T t = n.get();
        if (t == null) {
            size--;
            if (last == null) {
                nodes[index] = n.next();
            } else {
                last.setNext(n.next());
            }
            continue;
        }
        if (equals.test(t)) {
            Node<T> newn = replacement.apply(n);
            if (newn == null) {
                size--;
                if (last == null) {
                    nodes[index] = n.next();
                } else {
                    last.setNext(n.next());
                }
            } else if (newn != n) {
                newn.setNext(n.next());
                if (last == null) {
                    nodes[index] = newn;
                } else {
                    last.setNext(newn);
                }
            }
            resize();
            return t;
        }
        last = n;
    }
    resize();
    return null;
}
private void resize() {
    int s = getNewSize();
    if (s != nodes.length) {
        resize(s);
    }
}
private int getNewSize() {
    int s = nodes.length;
    while (size > s * growFactor && s < MAX_CAPACITY) {
        s <<= 1;
    }
    while (size < s * shrinkFactor && s > MIN_CAPACITY) {
        s >>= 1;
    }
    return s;
}
@SuppressWarnings("unchecked")
private void resize(int length) {
/** Extension of {@link Pool#Add(T)} which allows the caller to specify the
     * reference type to be used to store the object. If a matching object is found
     * with a different reference type its type is changed to {@code referenceType} if
     * {@code changeCurrent} returns true. If {@code referenceType} is null the pool's
     * reference type is used instead. */

Context

StackExchange Code Review Q#58093, answer score: 3

Revisions (0)

No revisions yet.