+[27159] AbstractHashedMap subclasses failed to clone() correctly +
+ ++MultiKey - Add getKey(index) and size() methods and make constructor public +
+-[26470] Javadoc - Add javadoc about requiring Comparable entries -[27159] Bug - AbstractHashedMap subclasses failed to clone() correctly +[26470] TreeBidiMap - Add javadoc about requiring Comparable entries +MultiKey - Add extra explanatations, examples and warnings +
diff --git a/src/java/org/apache/commons/collections/keyvalue/MultiKey.java b/src/java/org/apache/commons/collections/keyvalue/MultiKey.java index 9135a3571..ad01b27e8 100644 --- a/src/java/org/apache/commons/collections/keyvalue/MultiKey.java +++ b/src/java/org/apache/commons/collections/keyvalue/MultiKey.java @@ -25,14 +25,27 @@ import java.util.Arrays; * maps of maps. An example might be the need to lookup a filename by * key and locale. The typical solution might be nested maps. This class * can be used instead by creating an instance passing in the key and locale. + *+ * Example usage: + *
+ * // populate map with data mapping key+locale to localizedText + * Map map = new HashMap(); + * MultiKey multiKey = new MultiKey(key, locale); + * map.put(multiKey, localizedText); + * + * // later retireve the localized text + * MultiKey multiKey = new MultiKey(key, locale); + * String localizedText = (String) map.get(multiKey); + ** * @since Commons Collections 3.0 - * @version $Revision: 1.4 $ $Date: 2004/02/18 01:00:08 $ + * @version $Revision: 1.5 $ $Date: 2004/03/13 12:43:43 $ * * @author Howard Lewis Ship * @author Stephen Colebourne */ public class MultiKey implements Serializable { + // This class could implement List, but that would confuse it's purpose /** Serialisation version */ private static final long serialVersionUID = 4465448607415788805L; @@ -44,6 +57,9 @@ public class MultiKey implements Serializable { /** * Constructor taking two keys. + *
+ * The keys should be immutable + * If they are not then they must not be changed after adding to the MultiKey. * * @param key1 the first key * @param key2 the second key @@ -54,6 +70,9 @@ public class MultiKey implements Serializable { /** * Constructor taking three keys. + *
+ * The keys should be immutable + * If they are not then they must not be changed after adding to the MultiKey. * * @param key1 the first key * @param key2 the second key @@ -65,6 +84,9 @@ public class MultiKey implements Serializable { /** * Constructor taking four keys. + *
+ * The keys should be immutable + * If they are not then they must not be changed after adding to the MultiKey. * * @param key1 the first key * @param key2 the second key @@ -77,6 +99,9 @@ public class MultiKey implements Serializable { /** * Constructor taking five keys. + *
+ * The keys should be immutable + * If they are not then they must not be changed after adding to the MultiKey. * * @param key1 the first key * @param key2 the second key @@ -89,9 +114,14 @@ public class MultiKey implements Serializable { } /** - * Constructor taking an array of keys. + * Constructor taking an array of keys which is cloned. + *
+ * The keys should be immutable + * If they are not then they must not be changed after adding to the MultiKey. + *
+ * This is equivalent to new MultiKey(keys, true).
*
- * @param keys the array of keys
+ * @param keys the array of keys, not null
* @throws IllegalArgumentException if the key array is null
*/
public MultiKey(Object[] keys) {
@@ -99,20 +129,35 @@ public class MultiKey implements Serializable {
}
/**
- * Constructor taking an array of keys.
+ * Constructor taking an array of keys, optionally choosing whether to clone.
*
- * If the array is not copied, then it must not be modified. + * If the array is not cloned, then it must not be modified. + *
+ * This method is public for performance reasons only, to avoid a clone. + * The hashcode is calculated once here in this method. + * Therefore, changing the array passed in would not change the hashcode but + * would change the equals method, which is a bug. + *
+ * This is the only fully safe usage of this constructor, as the object array + * is never made available in a variable: + *
+ * new MultiKey(new Object[] {...}, false);
+ *
+ * + * The keys should be immutable + * If they are not then they must not be changed after adding to the MultiKey. * - * @param keys the array of keys - * @param makeCopy true to copy the array, false to assign it + * @param keys the array of keys, not null + * @param makeClone true to clone the array, false to assign it * @throws IllegalArgumentException if the key array is null + * @since Commons Collections 3.1 */ - protected MultiKey(Object[] keys, boolean makeCopy) { + public MultiKey(Object[] keys, boolean makeClone) { super(); if (keys == null) { throw new IllegalArgumentException("The array of keys must not be null"); } - if (makeCopy) { + if (makeClone) { this.keys = (Object[]) keys.clone(); } else { this.keys = keys; @@ -121,18 +166,18 @@ public class MultiKey implements Serializable { int total = 0; for (int i = 0; i < keys.length; i++) { if (keys[i] != null) { - if (i == 0) { - total = keys[i].hashCode(); - } else { - total ^= keys[i].hashCode(); - } + total ^= keys[i].hashCode(); } } hashCode = total; } + //----------------------------------------------------------------------- /** - * Gets a copy of the individual keys. + * Gets a clone of the array of keys. + *
+ * The keys should be immutable + * If they are not then they must not be changed. * * @return the individual keys */ @@ -140,6 +185,32 @@ public class MultiKey implements Serializable { return (Object[]) keys.clone(); } + /** + * Gets the key at the specified index. + *
+ * The key should be immutable. + * If it is not then it must not be changed. + * + * @param index the index to retrieve + * @return the key at the index + * @throws IndexOutOfBoundsException if the index is invalid + * @since Commons Collections 3.1 + */ + public Object getKey(int index) { + return keys[index]; + } + + /** + * Gets the size of the list of keys. + * + * @return the size of the list of keys + * @since Commons Collections 3.1 + */ + public int size() { + return keys.length; + } + + //----------------------------------------------------------------------- /** * Compares this object to another. *
diff --git a/src/test/org/apache/commons/collections/keyvalue/TestMultiKey.java b/src/test/org/apache/commons/collections/keyvalue/TestMultiKey.java index 31e72c573..56193e8b2 100644 --- a/src/test/org/apache/commons/collections/keyvalue/TestMultiKey.java +++ b/src/test/org/apache/commons/collections/keyvalue/TestMultiKey.java @@ -25,7 +25,7 @@ import junit.framework.TestSuite; /** * Unit tests for {@link org.apache.commons.collections.MultiKey}. * - * @version $Revision: 1.3 $ $Date: 2004/02/18 01:20:40 $ + * @version $Revision: 1.4 $ $Date: 2004/03/13 12:43:43 $ * * @author Stephen Colebourne */ @@ -58,8 +58,8 @@ public class TestMultiKey extends TestCase { super.tearDown(); } - - public void testConstructorsAndGet() throws Exception { + //----------------------------------------------------------------------- + public void testConstructors() throws Exception { MultiKey mk = null; mk = new MultiKey(ONE, TWO); Assert.assertTrue(Arrays.equals(new Object[] {ONE, TWO}, mk.getKeys())); @@ -75,13 +75,109 @@ public class TestMultiKey extends TestCase { mk = new MultiKey(new Object[] {THREE, FOUR, ONE, TWO}, false); Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, TWO}, mk.getKeys())); - - // don't do this! + } + + public void testConstructorsByArray() throws Exception { + MultiKey mk = null; Object[] keys = new Object[] {THREE, FOUR, ONE, TWO}; mk = new MultiKey(keys); Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, TWO}, mk.getKeys())); keys[3] = FIVE; // no effect Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, TWO}, mk.getKeys())); + + keys = new Object[] {}; + mk = new MultiKey(keys); + Assert.assertTrue(Arrays.equals(new Object[] {}, mk.getKeys())); + + keys = new Object[] {THREE, FOUR, ONE, TWO}; + mk = new MultiKey(keys, true); + Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, TWO}, mk.getKeys())); + keys[3] = FIVE; // no effect + Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, TWO}, mk.getKeys())); + + keys = new Object[] {THREE, FOUR, ONE, TWO}; + mk = new MultiKey(keys, false); + Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, TWO}, mk.getKeys())); + // change key - don't do this! + // the hashcode of the MultiKey is now broken + keys[3] = FIVE; + Assert.assertTrue(Arrays.equals(new Object[] {THREE, FOUR, ONE, FIVE}, mk.getKeys())); + } + + public void testConstructorsByArrayNull() throws Exception { + Object[] keys = null; + try { + new MultiKey(keys); + fail(); + } catch (IllegalArgumentException ex) {} + try { + new MultiKey(keys, true); + fail(); + } catch (IllegalArgumentException ex) {} + try { + new MultiKey(keys, false); + fail(); + } catch (IllegalArgumentException ex) {} + } + + public void testSize() { + Assert.assertEquals(2, new MultiKey(ONE, TWO).size()); + Assert.assertEquals(2, new MultiKey(null, null).size()); + Assert.assertEquals(3, new MultiKey(ONE, TWO, THREE).size()); + Assert.assertEquals(3, new MultiKey(null, null, null).size()); + Assert.assertEquals(4, new MultiKey(ONE, TWO, THREE, FOUR).size()); + Assert.assertEquals(4, new MultiKey(null, null, null, null).size()); + Assert.assertEquals(5, new MultiKey(ONE, TWO, THREE, FOUR, FIVE).size()); + Assert.assertEquals(5, new MultiKey(null, null, null, null, null).size()); + + Assert.assertEquals(0, new MultiKey(new Object[] {}).size()); + Assert.assertEquals(1, new MultiKey(new Object[] {ONE}).size()); + Assert.assertEquals(2, new MultiKey(new Object[] {ONE, TWO}).size()); + Assert.assertEquals(7, new MultiKey(new Object[] {ONE, TWO, ONE, TWO, ONE, TWO, ONE}).size()); + } + + public void testGetIndexed() { + MultiKey mk = new MultiKey(ONE, TWO); + Assert.assertSame(ONE, mk.getKey(0)); + Assert.assertSame(TWO, mk.getKey(1)); + try { + mk.getKey(-1); + fail(); + } catch (IndexOutOfBoundsException ex) {} + try { + mk.getKey(2); + fail(); + } catch (IndexOutOfBoundsException ex) {} + } + + public void testGetKeysSimpleConstructor() { + MultiKey mk = new MultiKey(ONE, TWO); + Object[] array = mk.getKeys(); + Assert.assertSame(ONE, array[0]); + Assert.assertSame(TWO, array[1]); + Assert.assertEquals(2, array.length); + } + + public void testGetKeysArrayConstructorCloned() { + Object[] keys = new Object[] {ONE, TWO}; + MultiKey mk = new MultiKey(keys, true); + Object[] array = mk.getKeys(); + Assert.assertTrue(array != keys); + Assert.assertTrue(Arrays.equals(array, keys)); + Assert.assertSame(ONE, array[0]); + Assert.assertSame(TWO, array[1]); + Assert.assertEquals(2, array.length); + } + + public void testGetKeysArrayConstructorNonCloned() { + Object[] keys = new Object[] {ONE, TWO}; + MultiKey mk = new MultiKey(keys, false); + Object[] array = mk.getKeys(); + Assert.assertTrue(array != keys); // still not equal + Assert.assertTrue(Arrays.equals(array, keys)); + Assert.assertSame(ONE, array[0]); + Assert.assertSame(TWO, array[1]); + Assert.assertEquals(2, array.length); } public void testHashCode() { @@ -92,6 +188,9 @@ public class TestMultiKey extends TestCase { Assert.assertTrue(mk1.hashCode() == mk1.hashCode()); Assert.assertTrue(mk1.hashCode() == mk2.hashCode()); Assert.assertTrue(mk1.hashCode() != mk3.hashCode()); + + int total = (0 ^ ONE.hashCode()) ^ TWO.hashCode(); + Assert.assertEquals(total, mk1.hashCode()); } public void testEquals() {