Wow. That's a lot of comments, all good ones. I'd hate to see you
sad, Henning, your cheery disposition lights up this project. :-)
One quick point. I want to re-iterate what I said in the JIRA note.
This is a bug that's been in Velocity a long time, in a fundamental
part of the caching routine. Nice job, Alexey, on finding this,
fixing it, and jumping through all the hoops to get the patch in.
Hope to see more.
I'll take responsibility to work with Alexey in cleaning this up.
Though to be fair, you are holding the patch to a higher standard than
the original. Of course the original had a bug.
And the typo in the copyright date is my fault. Legal issues are
really the responsibility of the committers :-)
WILL
On 9/27/06, Henning P. Schmiedehausen <[EMAIL PROTECTED]> wrote:
[EMAIL PROTECTED] writes:
(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).
>- private String methodName = "";
>+ protected String methodName = "";
[...]
>+ if (params.length == other.params.length &&
methodName.equals(other.getASTMethod().methodName))
I'm -0.5 on that patch. That coding style is not good (private is
there for a reason), the change to protected is not even needed,
because if you are an object of class "Foo", you can look at the
private fields of every other Foo, too:
public class Foo {
private final int value;
public Foo(final int value) { this.value = value; }
public void report(final Foo foo) {
System.out.println(foo.value);
}
}
public class Bar {
public static void main(String[] args) {
Foo foo1 = new Foo(23);
Foo foo2 = new Foo(42);
foo2.report(foo1);
}
}
Run "Bar" and you will get "23" as the result. foo2 is allowed to look
at the private value field of foo1.
If we need to look at member fields of any class, we add getters and
setters, not change the visibility of the member fields. It is the job
of the Java compiler (which does a very good job here) to optimize
that.
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.
The code doesn't adhere to our indent rules and the year date in the
copyright notice is wrong (should be 2006). I freely admit that I'm
picking nits 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.
The hash method uses the ^= method to add the parameters. This is a
really bad idea, because it detoriates the hash value. Consider two
arrays:
Class [] e1 = new Class [] { String.class, String.class };
Class [] e2 = new Class [] { Object.class, Object.class };
int result = 0;
for (int i = 0; i < params.length; ++i)
{
final Class param = params[i];
if (param != null)
{
result ^= param.hashCode();
}
}
That loop returns the same value for both arrays: 0 (The reason why is
left for the reader as an exercise :-) ).
Yes, it *is* hard to write a good hash function. That is why there is
so much literature about it.
E.g. http://java.sun.com/developer/Books/effectivejava/Chapter3.pdf
(In "Hard Core Java", the methods in the Jakarta commons are
explicitly mentioned as examples for a good hash code function. There
is a reason for this).
I know that "this is private code (if it is, why is MethodCacheKey
package protected and not private?)" and "it can never be called with
params == null". However, if you do, you get an NPE. If it is possible
to make that code robust, why not do it?
An unit test draft could look like this:
--- cut ---
package org.apache.velocity.runtime.parser.node;
import junit.framework.Test;
import junit.framework.TestSuite;
import org.apache.commons.lang.ArrayUtils;
import org.apache.velocity.runtime.parser.node.ASTMethod.MethodCacheKey;
import org.apache.velocity.test.BaseTestCase;
/*
* Copyright 2006 The Apache Software Foundation.
*
* Licensed under the Apache License, Version 2.0 (the "License")
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/**
* Test ASTMethod equals() and hashCode()
*
* @author <a href="mailto:[EMAIL PROTECTED]">Will Glass-Husain</a>
* @version $Id: ResourceCachingTestCase.java 447969 2006-09-19 21:00:14Z
henning $
*/
public class MethodKeyTestCase extends BaseTestCase
{
/**
* Default constructor.
*/
public MethodKeyTestCase(String name)
{
super(name);
}
public void setUp()
throws Exception
{
}
public static Test suite()
{
return new TestSuite(MethodKeyTestCase.class);
}
public void testEquals()
{
Class [] elements = new Class [] { Object.class };
ASTMethod m1 = new ASTMethod(23);
MethodCacheKey mck = m1.new MethodCacheKey(elements);
assertTrue(mck.equals(mck));
assertTrue(!mck.equals(null));
assertTrue(!mck.equals((MethodCacheKey) null));
}
public void testEqualsPathological()
{
Class [] elements = new Class [] {};
ASTMethod m1 = new ASTMethod(23);
MethodCacheKey mck = m1.new MethodCacheKey(elements);
assertTrue(mck.equals(mck));
assertTrue(!mck.equals(null));
assertTrue(!mck.equals((MethodCacheKey) null));
}
public void testEqualsPathological2()
{
Class [] elements = new Class [] {null};
ASTMethod m1 = new ASTMethod(23);
MethodCacheKey mck = m1.new MethodCacheKey(elements);
assertTrue(mck.equals(mck));
assertTrue(!mck.equals(null));
assertTrue(!mck.equals((MethodCacheKey) null));
}
public void testEqualsBad()
{
Class [] elements = ArrayUtils.EMPTY_CLASS_ARRAY;
ASTMethod m1 = new ASTMethod(23);
MethodCacheKey mck = m1.new MethodCacheKey(elements);
assertTrue(mck.equals(mck));
assertTrue(!mck.equals(null));
assertTrue(!mck.equals((MethodCacheKey) null));
}
public void testEqualsWorse()
{
Class [] elements = null;
ASTMethod m1 = new ASTMethod(23);
MethodCacheKey mck = m1.new MethodCacheKey(elements);
assertTrue(mck.equals(mck));
assertTrue(!mck.equals(null));
assertTrue(!mck.equals((MethodCacheKey) null));
}
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());
}
}
--- cut ---
I'd like the code to pass all of those tests. Yes, there is a number
of pathologic tests.
To make a medium length story short: That code would not pass my
personal QA. I'd like to get it reviewed and fixed. I will not insist
on it (that's why it is -0.5 and not -1 but it will make me sad if the
code stays as it is).
Best regards
Henning
--
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen INTERMETA GmbH
[EMAIL PROTECTED] +49 9131 50 654 0 http://www.intermeta.de/
RedHat Certified Engineer -- Jakarta Turbine Development -- hero for hire
Linux, Java, perl, Solaris -- Consulting, Training, Development
Social behaviour: Bavarians can be extremely egalitarian and folksy.
-- http://en.wikipedia.org/wiki/Bavaria
Most Franconians do not like to be called Bavarians.
-- http://en.wikipedia.org/wiki/Franconia
---------------------------------------------------------------------
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]
