Henning P. Schmiedehausen wrote:
> (This mail might sound a bit hash. It is not intended as such, but I
> want to make sure that we do get peer review with code going in, even
> if it is "just a small fix". So I intend to use this as an example
> case. Sorry 'bout that. This message is intended to criticize the
> code, not you or Alexey).
I agree with part of the critiques, and disagree with the other part.
>>- private String methodName = "";
>>+ protected String methodName = "";
ok
> I don't like the fact that there is object comparing (the Class
> elements of params should be compared using equals, not != ) in the
> code.
This is just waste of processor cycles.
The Class does not override the equals().
And it never be overriden.
> mainly because I don't like the implementation of
> the methods and don't believe in the handwaving that "this is faster
> than using EqualsBuilder and HashCodeBuilder because of Object
> creation". I haven't seen any numbers to prove or disprove that claim.
You want to compare the speed of
for (int i = 0; i < params.length; ++i) {
final Class param = params[i];
if (param != null) {
result ^= param.hashCode();
}
}
with the code in HashCodeBuilder:
public static int reflectionHashCode(
int initialNonZeroOddNumber,
int multiplierNonZeroOddNumber,
Object object,
boolean testTransients,
Class reflectUpToClass,
String[] excludeFields) {
if (object == null) {
throw new IllegalArgumentException("The object to build a hash code
for must not be null");
}
HashCodeBuilder builder = new HashCodeBuilder(initialNonZeroOddNumber,
multiplierNonZeroOddNumber);
Class clazz = object.getClass();
reflectionAppend(object, clazz, builder, testTransients, excludeFields);
while (clazz.getSuperclass() != null && clazz != reflectUpToClass) {
clazz = clazz.getSuperclass();
reflectionAppend(object, clazz, builder, testTransients,
excludeFields);
}
return builder.toHashCode();
}
private static void reflectionAppend(
Object object,
Class clazz,
HashCodeBuilder builder,
boolean useTransients,
String[] excludeFields) {
Field[] fields = clazz.getDeclaredFields();
List excludedFieldList = excludeFields != null ?
Arrays.asList(excludeFields) : Collections.EMPTY_LIST;
AccessibleObject.setAccessible(fields, true);
for (int i = 0; i < fields.length; i++) {
Field f = fields[i];
if (!excludedFieldList.contains(f.getName())
&& (f.getName().indexOf('$') == -1)
&& (useTransients || !Modifier.isTransient(f.getModifiers()))
&& (!Modifier.isStatic(f.getModifiers()))) {
try {
builder.append(f.get(object));
} catch (IllegalAccessException e) {
//this can't happen. Would get a Security exception instead
//throw a runtime exception in case the impossible happens.
throw new InternalError("Unexpected
IllegalAccessException");
}
}
}
}
and so on ?
> The hash method uses the ^= method to add the parameters. This is a
> really bad idea, because it detoriates the hash value.
ok, the hash function could be better.
> I know that "this is private code (if it is, why is MethodCacheKey
> package protected and not private?)"
This is the question to the original author of this class.
I just wanted to minimize changes in my patch.
> An unit test draft could look like this:
...
> public void testHashCode()
> {
> Class [] e1 = new Class [] { String.class, String.class };
> Class [] e2 = new Class [] { Object.class, Object.class };
> ASTMethod m1 = new ASTMethod(23);
> MethodCacheKey mck1 = m1.new MethodCacheKey(e1);
> MethodCacheKey mck2 = m1.new MethodCacheKey(e2);
> assertTrue(mck1.hashCode() != mck2.hashCode());
> }
I do not agree with this test. It is not required (and is not always
possible) that different objects return different hashcodes.
--
Best regards,
Alexey mailto:[EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
