5
\$\begingroup\$

Below is a function that determines if Class B uses Class A.

Currently, it tests for:

  1. Fields
  2. Superclass
  3. Constructors
  4. Methods
private boolean uses(Class<?> b, Class<?> a){
    // Test for Declared field
    for(Field f:b.getDeclaredFields()){
        if(f.getGenericType().equals(a))
            return true;
    } 
    // Test for Inherietence
    if(b.getSuperclass().getName().equals(a.getName()))
        return true;

    // Test for constructors
    for(Constructor<?> c: b.getDeclaredConstructors()){
        for(Class<?> p : c.getParameterTypes())
            if(p.getName().equals(a.getName()))
                return true;
    }
    // Test for methods 
    for(Method m:b.getDeclaredMethods()){
        for(Class<?> p:m.getParameterTypes())
            if(p.getName().equals(a.getName()))
                return true;
    }

    return false;
}

Is there a better way to write this function?

Is the name of the function terrible?

Are there any bugs in the code?

Is my test holistic, that is, did I test for everything?

\$\endgroup\$
  • 1
    \$\begingroup\$ possible bug. Using getSuperclass() to check the inheritance is not enough, because it will return either the direct parent class or null in some cases. isAssignableFrom(Class) should be used instead. \$\endgroup\$ – Antot Apr 23 '16 at 22:18
  • \$\begingroup\$ "did I test for everything?" – No. Like almost all static analysis, this is basically equivalent to solving the Halting Problem, and/or Rice's Theorem. \$\endgroup\$ – Jörg W Mittag Apr 24 '16 at 11:17
  • \$\begingroup\$ Hey by the way, I just noticed you have not accepted any answer. If you could do that, it would close this question and it would give both you and the one who provided that answer some positive reputation. \$\endgroup\$ – ZeroOne Jul 29 '16 at 11:19
5
\$\begingroup\$

Is my test holistic, that is, did I test for everything?

Currently, this is what you're testing for:

This is what you are not testing for:

  • Inherited fields: they are not returned by the call to getDeclaredFields(), so fields inherited from superclasses are not considered;
  • The method return type: you are just checking for the type of the parameters but not the return type with getGenericReturnType();
  • Annotations on the method (getAnnotations()), on the method parameters (getParameterAnnotations()) or on the method return type (getAnnotatedReturnType());
  • Annotations on the class itself with getAnnotations();
  • Exceptions declared to be thrown by the methods with getGenericExceptionTypes(), in the same way, the exceptions declared to be thrown by the declared constructors are also not considered;
  • All the hierarchy of the class: you are only testing for the potential superclass, but not for the potential implemented interfaces or the super-superclasses;
  • If the given class is an array type, you are not testing its component type with getComponentType().
  • For the generic types returned by getDeclaredFields() or getGenericReturnType() for example, you can access the actual type argument by casting the type to ParameterizedType and call getActualTypeArguments(). This will return an array of all the type argument of the parameterized type. For example, if a declared field is a Map<String, Integer>, it will return the array [class java.lang.String, class java.lang.Integer].

Are there any bugs in the code?

Yes, there is a potential bug: getSuperclass can return null:

If this Class represents either the Object class, an interface, a primitive type, or void, then null is returned

so you need to take care about handling that. Your current code is

if(b.getSuperclass().getName().equals(a.getName()))

which could throw a NullPointerException.


Other comments:

  • Make sure to always use curly braces, even if it is not strictly needed here. It will make the code less error-prone for the future.
  • This only uses the Reflection API. Determining whether a given class A truly uses a given class B or not would require going into the byte-code to see if there are any references through chained method calls, import statements or local variables.
\$\endgroup\$
  • \$\begingroup\$ Question for b.getSuperclass(). It returns class java.lang.Object. What do you mean null?? \$\endgroup\$ – None Apr 23 '16 at 22:39
  • \$\begingroup\$ @user3773246 getSuperclass() is documented to return null if this class is java.lang.Object yes. Test with System.out.println(Object.class.getSuperclass());, you'll see :). \$\endgroup\$ – Tunaki Apr 23 '16 at 22:40
  • \$\begingroup\$ So write if(b.getSuperclass().equals(null)) return false; ?? \$\endgroup\$ – None Apr 23 '16 at 22:42
  • 1
    \$\begingroup\$ @user3773246 No, that will throw a NullPointerException since you're calling equals on a null reference. You need to check for null with != null. \$\endgroup\$ – Tunaki Apr 23 '16 at 22:43
  • \$\begingroup\$ Like previous answer said, I no longer will be comparing getName(). \$\endgroup\$ – None Apr 23 '16 at 22:44
4
\$\begingroup\$

First of all, your comments are completely superfluous. Consider this wisdom from Clean Code by Uncle Bob:

So when you find yourself in a position where you need to write a comment, think it through and see whether there isn’t some way to turn the tables and express yourself in code. Every time you express yourself in code, you should pat yourself on the back. Every time you write a comment, you should grimace and feel the failure of your ability of expression

In your case you can turn your code more expressive like this:

private boolean uses(Class<?> b, Class<?> a){
    if (testForDeclaredFields(b, a)) return true;
    if (testForInheritance(b, a)) return true;
    if (testForConstructors(b, a)) return true;
    if (testForMethods(b, a)) return true;
    return false;
}

Second, you should write proper unit tests for your code. That's a good way of verifying your code works.

Third, you are comparing the class name strings, when you could just compare the Class objects: instead of p.getName().equals(a.getName()) you can just say p.equals(a).

And as was already discussed in the comment section of your question, your code does not cover all the uses cases. It's missing at least local variables, inherited methods, and method return types.

\$\endgroup\$
  • 1
    \$\begingroup\$ Actually, it would be even better to test equality between classes with ==. It will take care of potential nulls in the process. \$\endgroup\$ – Tunaki Apr 23 '16 at 22:47
  • \$\begingroup\$ How is testing for reference better? \$\endgroup\$ – None Apr 23 '16 at 22:48
  • \$\begingroup\$ @user3773246 See the Stack Overflow answer I linked to. \$\endgroup\$ – Tunaki Apr 23 '16 at 22:49
  • \$\begingroup\$ Luckily in this case we know that p cannot be null because all parameters must have a type. \$\endgroup\$ – ZeroOne Apr 23 '16 at 23:00
  • \$\begingroup\$ This would be far better as a single line returning a || b || c ... \$\endgroup\$ – Software Engineer Apr 28 '16 at 1:40
0
\$\begingroup\$

So, if I have a simple class like this:

class NoName {
    public void thing() {
        BigDecimal bd = BigDecimal.ZERO;
        System.out.println(bd.toPlainString());
    }
}

Would your code be likely to discover that I'm using BigDecimal? How about System? Or, PrintStream?

Actually, you're checking if I'm using a given class in my interface, not in my implementation. I could be using the class you're searching for a thousand times and you'd still not know.

You're not only missing these local variables and uses, but you're also missing method return types, anything inherited, and generics on each class of object you've already considered including the class itself.

If you're looking for a has-a or a uses-a relationship you'll have to work a lot harder than this to get it right. You would probably have to decompile the bytecode or use the Java platform debugger architecture (JPDA).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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