Hi Alexey,
Thanks for the note. I made MethodCacheKey a public inner class so
that we could access it from the unit test. Couldn't figure out a
better way to do this, though suggestions are welcome.
I know, the NPE checks seem a little silly. Still, the checks are
cheap. I worry that the code may be reused in the future and the
caveats forgotten. I'm fine with checking them in the constructor,
not a bad idea.
Best,
WILL
On 9/28/06, Alexey Panchenko <[EMAIL PROTECTED]> wrote:
Will Glass-Husain wrote:
> * I applied Alexey's modified patch. (much appreciated)
You missed one point - MethodCacheKey is a static class in my patch.
The methodName is copied from the ASTMethod into the MethodCacheKey,
so the MethodCacheKey doesn't need access to it's parent class.
> * I applied Alexey's new hashCode routine. seems reasonable to me.
> * I added NPE checks to equals and hash code. While Alexey's point
> these are never needed is a good one, I worry about the consequence of
> adding code in the future that uses this class in a new way.
Your added "if (methodName != null)" in hashCode(), however there is
no such check in equals(). This is inconsistent.
Btw, methodName is never null - it is parsed from the template and I
can't imagine such a template.
And actually, I don't quite understand your concerns about the nulls.
- The assert() is a standard and compact way to express the
requirements for the variable values. If somebody starts using this
class the way it's not supposed to be used - the asserts would fail.
This class is internal to Velocity, so somebody == Velocity developers
only.
- If you believe null should be accepted as a parameter the simplier
way is to check it in the constructor:
this.methodName = methodName != null ? methodName : StringUtils.EMPTY;
this.params = params != null ? params : ArrayUtils.EMPTY_CLASS_ARRAY;
So the rest of the code could be much simplier.
> * Similarly, I changed == to equals() in the equals() method. (or
> rather, != to !equals)
> * Finally, I added a version of Hennings' equals() unit test.
> Normally I'd argue a unit test for equals() is overkill. But here,
> the equals() method is the main point of MethodCacheKey (and is the
> feature that was incorrect in the previous version).
> Best, WILL
> On 9/28/06, Henning Schmiedehausen <[EMAIL PROTECTED]> wrote:
>> Hi Alexey,
>>
>> thanks a lot for your patch. I like it and we should apply it. I very
>> much appreciate that you understood my mail as peer review and not
>> critique per se.
>>
>> [...]
>> > This is just waste of processor cycles.
>> > The Class does not override the equals().
>>
>> Hm. It does not for the Sun JDK and I'm pretty sure that it will not be
>> overridden by any other JDK, but this is IMHO not guaranteed.
>>
>> One of my "Java memes" is "thou shalt never compare two objects with ==
>> or !=". A class object is definitely an object. Yes, the default
>> implementation of equals() inside Object is '=='.
>>
>> However, it is the job of the compiler to optimize that away, not of the
>> developer to second guess it. So for the sake of being anal, I'd like to
>> see equals() here. :-)
>>
>> > And it never be overriden.
>>
>> Sure? /me thinks of weird class loader things and runtime byte code
>> 'improvements'... :-) But again, I'm splitting hairs here...
>>
>> >
>> >> 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
>>
>> [... HCB vs. hand-rolled code removed ...]
>>
>> Yes, because I wouldn't use the Reflection code but add the attributes
>> explicitly and again, I will not second guess the compiler. I'll try to
>> get some numbers tonight...
>>
>> [...]
>>
>> >> 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.
>>
>> Yes, you are right. That is a leftover and should not be there. As I
>> said, this was a draft. (I do know that the smallest legal
>> implementation of hashCode() is public int hashCode() { return 42; } ;-) )
>>
>> Best regards
>> Henning
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [EMAIL PROTECTED]
>> For additional commands, e-mail: [EMAIL PROTECTED]
>>
>>
--
Best regards,
Alexey mailto:[EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
--
Forio Business Simulations
Will Glass-Husain
[EMAIL PROTECTED]
www.forio.com
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
