Re[2]: MethodCacheKey patch (was: svn commit: r449347)

[Alexey Panchenko <al...@olmisoft.com>]

Will Glass-Husain wrote:

> 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.

@@ -320,7 +341,7 @@

      * public access (and complete constructor) for unit test 
      * purposes.
      */
-    public class MethodCacheKey
+    public static class MethodCacheKey
     {
         private final String methodName;  
         private final Class[] params;

and remove "ASTMethod m1 = new ASTMethod(23);" lines from the test.

> 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 <al...@olmisoft.com> 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 <hp...@intermeta.de> 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: velocity-dev-unsubscribe@jakarta.apache.org
>> >> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>> >>
>> >>
>>
>>
>>
>>
>>
>> --
>> Best regards,
>>  Alexey                            mailto:alex+news@olmisoft.com
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
>> For additional commands, e-mail: velocity-dev-help@jakarta.apache.org
>>
>>





-- 
Best regards,
 Alexey                            mailto:alex+news@olmisoft.com


---------------------------------------------------------------------
To unsubscribe, e-mail: velocity-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: velocity-dev-help@jakarta.apache.org