Below is a function that determines if Class B uses Class A.
Currently, it tests for:
- Fields
- Superclass
- Constructors
- 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?
3 Answers 3
Is my test holistic, that is, did I test for everything?
Currently, this is what you're testing for:
- Declared fields on the class with
getDeclaredFields()
; - A superclass with
getSuperclass()
; - The parameters of the declared constructors with
getDeclaredConstructors()
; - The parameters of the declared methods with
getDeclaredMethods()
.
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()
orgetGenericReturnType()
for example, you can access the actual type argument by casting the type toParameterizedType
and callgetActualTypeArguments()
. This will return an array of all the type argument of the parameterized type. For example, if a declared field is aMap<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 theObject
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.
-
\$\begingroup\$ Question for b.getSuperclass(). It returns class java.lang.Object. What do you mean null?? \$\endgroup\$user102889– user1028892016年04月23日 22:39:29 +00:00Commented Apr 23, 2016 at 22:39
-
\$\begingroup\$ @user3773246
getSuperclass()
is documented to returnnull
if this class isjava.lang.Object
yes. Test withSystem.out.println(Object.class.getSuperclass());
, you'll see :). \$\endgroup\$Tunaki– Tunaki2016年04月23日 22:40:51 +00:00Commented Apr 23, 2016 at 22:40 -
\$\begingroup\$ So write if(b.getSuperclass().equals(null)) return false; ?? \$\endgroup\$user102889– user1028892016年04月23日 22:42:53 +00:00Commented Apr 23, 2016 at 22:42
-
1\$\begingroup\$ @user3773246 No, that will throw a
NullPointerException
since you're callingequals
on a null reference. You need to check for null with!= null
. \$\endgroup\$Tunaki– Tunaki2016年04月23日 22:43:56 +00:00Commented Apr 23, 2016 at 22:43 -
\$\begingroup\$ Like previous answer said, I no longer will be comparing getName(). \$\endgroup\$user102889– user1028892016年04月23日 22:44:00 +00:00Commented Apr 23, 2016 at 22:44
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.
-
1\$\begingroup\$ Actually, it would be even better to test equality between classes with
==
. It will take care of potentialnull
s in the process. \$\endgroup\$Tunaki– Tunaki2016年04月23日 22:47:47 +00:00Commented Apr 23, 2016 at 22:47 -
\$\begingroup\$ How is testing for reference better? \$\endgroup\$user102889– user1028892016年04月23日 22:48:51 +00:00Commented Apr 23, 2016 at 22:48
-
\$\begingroup\$ @user3773246 See the Stack Overflow answer I linked to. \$\endgroup\$Tunaki– Tunaki2016年04月23日 22:49:29 +00:00Commented Apr 23, 2016 at 22:49
-
\$\begingroup\$ Luckily in this case we know that
p
cannot be null because all parameters must have a type. \$\endgroup\$ZeroOne– ZeroOne2016年04月23日 23:00:59 +00:00Commented Apr 23, 2016 at 23:00 -
\$\begingroup\$ This would be far better as a single line returning a || b || c ... \$\endgroup\$Software Engineer– Software Engineer2016年04月28日 01:40:04 +00:00Commented Apr 28, 2016 at 1:40
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).
getSuperclass()
to check the inheritance is not enough, because it will return either the direct parent class ornull
in some cases.isAssignableFrom(Class)
should be used instead. \$\endgroup\$