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 <[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] >> >> -- Best regards, Alexey mailto:[EMAIL PROTECTED] --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]