Determine the class hierarchy of a class

Posted on

Problem

My only concern with the following class is the line hierarchy.trimToSize() in determineHierarchy, because it is not necessary for determining the class hierarchy. However, putting it in the constructor clutters the code more than my current solution. Any recommendations or should I just live with it?

Since this class is completely finished and tested, I also look for any other helpful comments. The target version is 1.8.

/** The class hierarchy of a class. */
public class ClassHierarchy {

    private final List<Class<?>> hierarchy;

    /**
     * Creates a ClassHierarchy instance for the given class {@literal clazz}.
     * 
     * @param clazz
     *            Class for which the class hierarchy should be determined. Must not be null and must refer to a class
     *            type.
     * 
     * @throws IllegalArgumentException
     *             if {@literal clazz} does not refer to a class type
     * @throws NullPointerException
     *             if {@literal clazz} is null
     */
    public ClassHierarchy(Class<?> clazz) {
        validateConstructorArgument(clazz);
        hierarchy = Collections.unmodifiableList(determineHierarchy(clazz));
    }

    /**
     * Returns the class hierarchy as an immutable list.
     * 
     * <p>
     * The classes are ordered, starting with the class for which the instance was created and ending with
     * {@code Object.class}.
     * 
     * @return the class hierarchy as an immutable list.
     */
    public List<Class<?>> getHierarchy() {
        return hierarchy;
    }

    private static void validateConstructorArgument(Class<?> clazz) {
        Objects.requireNonNull(clazz, "clazz must not be null");

        if (clazz.isEnum() || clazz.isAnnotation() || clazz.isInterface() || clazz.isArray() || clazz.isPrimitive()) {
            throw new IllegalArgumentException("clazz must refer to a class type");
        }
    }

    private static ArrayList<Class<?>> determineHierarchy(Class<?> clazz) {
        ArrayList<Class<?>> hierarchy = new ArrayList<>();
        for (Class<?> currentClass = clazz; currentClass != null; currentClass = currentClass.getSuperclass()) {
            hierarchy.add(currentClass);
        }
        hierarchy.trimToSize();
        return hierarchy;
    }

}

Solution

The one comment I have is that I find your if statement to be really hard to read:

if (clazz.isEnum() || clazz.isAnnotation() || clazz.isInterface() || clazz.isArray() || clazz.isPrimitive()) 

I would at least put that into a small helper to throw that exception:

private void checkClassRefersToClassType()

Your class breaks Single Responsibility Principle combining doing real work with result caching in hierarchy variable.

Why do you know you need caching at all? It looks like premature optimization here.

I would remove this variable at all and move the call of determineHierarchy inside of getHierarchy.

My only concern with the following class is the line hierarchy.trimToSize() in determineHierarchy

You could replace the ArrayList through a LinkedList. While the ArrayList reserves space, the LinkedList stores only what it owns. Additionally I would change the for-loop to a (in this case) more readable while-loop.

private static List<Class<?>> determineHierarchy(Class<?> clazz) {
  List<Class<?>> hierarchy = new LinkedList<>();

  Class<?> currentClass = clazz;
  while (currentClass != null) {
    hierarchy.add(currentClass);
    currentClass = currentClass.getSuperclass();
  }

  return hierarchy;
}

However, putting it in the constructor clutters the code more than my current solution

For that I would create a public static factory method. This method contains all the validation logic while the constructor can be private and dump.

private ClassHierarchy(List<Class<?>> clazz) {
  hierarchy = clazz;
}

public static ClassHierarchy of(Class<?> clazz) {
  Objects.requireNonNull(clazz, "clazz must not be null");

  if (isProperClass(clazz))
    return new ClassHierarchy(determineHierarchy(clazz));

  throw new IllegalArgumentException("clazz must refer to a class type");
}

Any recommendations or should I just live with it?

As already mentioned a method for the validation would be a nice to have.
Maybe it could like:

private static boolean isProperClass(Class<?> clazz) {
  return Stream.of(clazz)
               .filter(Class::isEnum)
               .filter(Class::isAnnotation)
               .filter(Class::isInterface)
               .noneMatch(Class::isPrimitive);
}

Leave a Reply

Your email address will not be published. Required fields are marked *