6
\$\begingroup\$

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;
    }

}
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

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()
\$\endgroup\$
5
  • \$\begingroup\$ It is unfortunate that there is a method for every case except for proper classes. So I could implement it myself (isProperClass). I still would throw the exception in validateConstructorArgument though. \$\endgroup\$ Sep 5, 2016 at 18:58
  • 1
    \$\begingroup\$ I agree, I dont see a nice way for this check; but I think having it in its own method makes it "sense" more clear. \$\endgroup\$
    – GhostCat
    Sep 6, 2016 at 3:25
  • \$\begingroup\$ Isn't it already pretty much in its' own method? \$\endgroup\$ Jun 20, 2018 at 19:46
  • \$\begingroup\$ @SimonForsberg When you follow "clean code" to the extreme as taught by Robert Martin, you could either move the whole if thing into a method, or at least put the 4 conditions into some doesClassReferToClassType() helper. \$\endgroup\$
    – GhostCat
    Jun 21, 2018 at 7:17
  • \$\begingroup\$ Yeah, a boolean method that returns clazz.isEnum() || clazz.isAnnotation() || clazz.isInterface() || clazz.isArray() || clazz.isPrimitive() would be nice. \$\endgroup\$ Jun 21, 2018 at 11:56
2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ If I remove the variable, I no longer need instances of that class, every method could be static and I would want to come up with a different name for it. \$\endgroup\$ Sep 5, 2016 at 18:43
  • 1
    \$\begingroup\$ @PhilippMatthiasSchäfer Disagree. Object of your class is hierarchy representation for specific class. You may have different hierarchies for different classes at the same time. \$\endgroup\$ Sep 5, 2016 at 18:51
  • \$\begingroup\$ If hierarchy representaiton is the object of the class (which it is), I need a variable to hold the hierarchy. I would then have to move the code determining that hierarchy outside (e.g. a ClassHierarchyFactory) which would only consist of static methods. That might be a way to go. \$\endgroup\$ Sep 5, 2016 at 18:55
  • \$\begingroup\$ @PhilippMatthiasSchäfer In reality you don't need any variables. Think about your ClassHierarchy as a view to your clazz. Only one thing is impotent - API of your ClassHierarchy. If you getHierarchy will be called thousand times per seconds and on a base of profiler result you'll decide it's a bottleneck, only after this it's needed to do some caching. \$\endgroup\$ Sep 5, 2016 at 19:07
  • \$\begingroup\$ @PhilippMatthiasSchäfer and don't use static methods without really significant reason. Read this first yegor256.com/2014/05/05/oop-alternative-to-utility-classes.html \$\endgroup\$ Sep 5, 2016 at 19:09
0
\$\begingroup\$

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);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.