3
\$\begingroup\$

A while ago I asked for a code review of my automatically keyed map and I have recently had the time to get around to writing some unit tests for it using JUnit4.

The test ensures that all of the "primary" functionality works, i.e. the things that could actually go wrong. The reason I don't test every method is that half of them forward their implementation to the map being wrapped.

Here is my current code (thanks to @Tunaki and @h.j.k. for their help in improving my old code):

AutoKeyedMap.java

import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

/**
 * A {@link Map} that is automatically keyed.
 *
 * @param <K> the key type for the map
 * @param <V> the value type for the map
 */
public abstract class AutoKeyedMap<K, V> implements Map<K, V> {
    private final Map<K, V> map;
    private boolean preventOverwrite;

    /**
     * Constructs a new automatically keyed map that wraps the provided map.
     *
     * @param map the map to wrap
     * @param preventOverwrite prevent overwriting of existing keys
     */
    public AutoKeyedMap(final Map<K, V> map, final boolean preventOverwrite) {
        this.map = map;
        this.preventOverwrite = preventOverwrite;
    }

    /**
     * Constructs a new automatically keyed map that wraps a {@link HashMap}.
     *
     * @param preventOverwrite prevent overwriting of existing keys
     */
    public AutoKeyedMap(final boolean preventOverwrite) {
        this(new HashMap<K, V>(), preventOverwrite);
    }

    /**
     * Constructs a new automatically keyed map that wraps the provided map.
     *
     * @param map the map to wrap
     */
    public AutoKeyedMap(final Map<K, V> map) {
        this(map, false);
    }

    /**
     * Constructs a new automatically keyed map that wraps a {@link HashMap}.
     */
    public AutoKeyedMap() {
        this(new HashMap<K, V>());
    }

    /**
     * Returns the key for the provided value.
     *
     * <p>
     * This method looks up or calculates a key for the provided value.
     *
     * It has no implementation by default and therefore it's workings are implementation dependant.
     * </p>
     *
     * @param value the value to get the key for
     *
     * @return the key for the provided value
     */
    public abstract K getKey(final V value);

    /**
     * Returns whether or not the map prevents overwriting of keys.
     *
     * @return whether or not the map prevents overwriting of keys
     */
    public final boolean isOverwritable() {
        return !preventOverwrite;
    }

    /**
     * Clears the map.
     *
     * @see Map#clear()
     */
    public final void clear() {
        map.clear();
    }

    /**
     * Returns whether or not the wrapped map contains the provided key.
     *
     * @param key the key to check
     *
     * @return whether or not the wrapped map contains the provided key
     *
     * @see Map#containsKey(Object)
     */
    public final boolean containsKey(final Object key) {
        return map.containsKey(key);
    }

    /**
     * Returns whether or not the wrapped map contains the provided value.
     *
     * @param value the value to check
     *
     * @return whether or not the wrapped map contains the provided value
     *
     * @see Map#containsValue(Object)
     */
    public final boolean containsValue(final Object value) {
        return map.containsValue(value);
    }

    /**
     * Returns the entry set for the wrapped map.
     *
     * @return the entry set for the wrapped map
     *
     * @see Map#entrySet()
     */
    public final Set<Map.Entry<K, V>> entrySet() {
        return map.entrySet();
    }

    /**
     * Returns whether or not the wrapped map is equal to the provided object.
     *
     * @param o the object to check
     *
     * @return whether or not the wrapped map is equal to the provided object
     *
     * @see Map#equals(Object)
     */
    public final boolean equals(final Object o) {
        return map.equals(o);
    }

    /**
     * Returns the value for the provided key.
     *
     * @param key the key to get the value for
     *
     * @return the value for the provided key
     *
     * @see Map#get(Object)
     */
    public final V get(final Object key) {
        return map.get(key);
    }

    /**
     * Returns the hash code of the wrapped map.
     *
     * @return the hash code of the wrapped map
     *
     * @see Map#hashCode()
     */
    public final int hashCode() {
        return map.hashCode();
    }

    /**
     * Returns whether or not the wrapped map is empty.
     *
     * @return whether or not the wrapped map is empty.
     *
     * @see Map#isEmpty()
     */
    public final boolean isEmpty() {
        return map.isEmpty();
    }

    /**
     * Returns the key set of the wrapped map.
     *
     * @return the key set of the wrapped map
     *
     * @see Map#keySet()
     */
    public final Set<K> keySet() {
        return map.keySet();
    }

    /**
     * Puts a key/value pair in the wrapped map.
     *
     * <p>
     * If the key is not <code>null</code> then this method will throw an {@link UnsupportedOperationException}.
     *
     * This is due to the fact that the key is automatically derived by {@link #getKey(Object)} and should not be
     * provided manually.
     * </p>
     *
     * <p>
     * If the key derived by {@link #getKey(Object)} already exists in the map and <code>preventOverwrite</code> is
     * <code>true</code> (i.e. {@link #isOverwritable()} returns <code>false</code>) then this method will throw
     * an {@link IllegalStateException}.
     * </p>
     *
     * @param key the key for the key/value pair
     * @param value the value for the key/value pair
     *
     * @return the previous value associated with the provided key
     *
     * @see Map#put(Object, Object)
     */
    public final V put(final K key, final V value) {
        if (key != null) {
            throw new UnsupportedOperationException("cannot put value with non-null key");
        }

        final K generatedKey = getKey(value);

        if (preventOverwrite && map.containsKey(generatedKey)) {
            throw new IllegalStateException("derived key already exists");
        }

        return map.put(generatedKey, value);
    }

    /**
     * Puts all of the key/values pairs from the provided map into the wrapped map.
     *
     * @param m the map to retrieve the key/value pairs from
     *
     * @see #put(Object, Object)
     * @see Map#putAll(Map)
     */
    public final void putAll(final Map<? extends K, ? extends V> m) {
        for (final Map.Entry<? extends K, ? extends V> e : m.entrySet()) {
            put(e.getKey(), e.getValue());
        }
    }

    /**
     * Removes the value with the provided key.
     *
     * @param key the key of the value to remove
     *
     * @return the previous value for the provided key
     *
     * @see Map#remove(Object)
     */
    public final V remove(final Object key) {
        return map.remove(key);
    }

    /**
     * Returns the size of the wrapped map.
     *
     * @return the size of the wrapped map
     *
     * @see Map#size()
     */
    public final int size() {
        return map.size();
    }

    /**
     * Returns the values of the wrapped map.
     *
     * @return the values of the wrapped map
     *
     * @see Map#values()
     */
    public final Collection<V> values() {
        return map.values();
    }
}

AutoKeyedMapTest.java

import java.util.HashMap;
import java.util.Map;

import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

/**
 * Tests for {@link AutoKeyedMap}.
 *
 * @see AutoKeyedMap
 */
public final class AutoKeyedMapTest {
    private static final String TEST_VALUE = "Value";

    /**
     * Tests for {@link AutoKeyedMap}.
     *
     * @see AutoKeyedMap
     */
    public AutoKeyedMapTest() { }

    /**
     * Tests putting a value into the map.
     *
     * <p>
     * This test puts a value in the map with a {@code null} key. It then checks to ensure that the key returned by
     * {@link AutoKeyedMap#getKey(Object)} is correct for the provided value.
     * </p>
     *
     * @see AutoKeyedMap#put(Object, Object)
     * @see AutoKeyedMap#getKey(Object)
     */
    @Test
    public void testPut() {
        final AutoKeyedMap<Integer, Object> autoKeyedMap = new HashCodeMap<>(new HashMap<Integer, Object>());

        // Add the test value to the map with a null key.
        autoKeyedMap.put(null, TEST_VALUE);

        // Get the generated key from the map.
        final Integer key = autoKeyedMap.getKey(TEST_VALUE);

        // Ensure that the key is correct.
        assertEquals(autoKeyedMap.get(key), TEST_VALUE);
    }

    /**
     * Tests putting a value into the map with {@code preventOverwrite} set to {@code true}.
     *
     * <p>
     * This test first creates a map with {@code preventOverwrite} set to {@code true}. It then puts a value in the
     * map with a {@code null} key and checks to ensure that the key returned by
     * {@link AutoKeyedMap#getKey(Object)} is correct for the provided value.
     * </p>
     *
     * @see AutoKeyedMap#put(Object, Object)
     * @see AutoKeyedMap#getKey(Object)
     */
    @Test
    public void testPutPreventOverwrite() {
        final AutoKeyedMap<Integer, Object> autoKeyedMap = new HashCodeMap<>(new HashMap<Integer, Object>(), true);

        // Add the test value to the map with a null key.
        autoKeyedMap.put(null, TEST_VALUE);

        // Get the generated key from the map.
        final Integer key = autoKeyedMap.getKey(TEST_VALUE);

        // Ensure that the key is correct.
        assertEquals(autoKeyedMap.get(key), TEST_VALUE);
    }

    /**
     * Tests putting an existing value into the map.
     *
     * <p>
     * This test puts a value in the map with a {@code null} key twice. It stores the return value from
     * {@link AutoKeyedMap#put(Object, Object)} on the second call and compares the value to the first value added
     * to the map. This is done to ensure that {@link AutoKeyedMap#put(Object, Object)} returns the previous value
     * for the generated key.
     * </p>
     *
     * @see AutoKeyedMap#put(Object, Object)
     * @see AutoKeyedMap#getKey(Object)
     */
    @Test
    public void testPutExisting() {
        final AutoKeyedMap<Integer, Object> autoKeyedMap = new HashCodeMap<>(new HashMap<Integer, Object>());

        final Object firstObject = new FakeHashCodeObject(0);
        final Object secondObject = new FakeHashCodeObject(0);

        autoKeyedMap.put(null, firstObject);

        // Put the second object in with the same automatically generated key (i.e. same hashCode value).
        final Object originalValue = autoKeyedMap.put(null, secondObject);

        // Ensure that put has returned the first object (i.e. the original value for the generated key).
        assertTrue(originalValue.equals(firstObject));

        // Get the generated key for the second object.
        final Integer key = autoKeyedMap.getKey(secondObject);

        // Ensure that the key is correct for the second object.
        assertEquals(autoKeyedMap.get(key), secondObject);
    }

    /**
     * Tests putting an existing value into the map with {@code preventOverwrite} set to {@code true}.
     *
     * <p>
     * This test first creates a map with {@code preventOverwrite} set to {@code true}. It then puts a value in the
     * map with a {@code null} key twice. This should throw an {@link IllegalStateException}, as
     * {@code preventOverwrite} ensures that a key cannot be overwritten.
     * </p>
     *
     * @see AutoKeyedMap#put(Object, Object)
     */
    @Test(expected = IllegalStateException.class)
    public void testPutExistingPreventOverwrite() {
        final Map<Integer, Object> autoKeyedMap = new HashCodeMap<>(new HashMap<Integer, Object>(), true);

        autoKeyedMap.put(null, TEST_VALUE);
        autoKeyedMap.put(null, TEST_VALUE);
    }

    /**
     * Tests putting a value into the map with a non-null key.
     *
     * <p>
     * This test puts a value into the map with a non-null key. It should throw an
     * {@link UnsupportedOperationException}, as the key is automatically generated in an {@link AutoKeyedMap}.
     * </p>
     *
     * @see AutoKeyedMap#put(Object, Object)
     */
    @Test(expected = UnsupportedOperationException.class)
    public void testPutNonNullKey() {
        final Map<Integer, Object> autoKeyedMap = new HashCodeMap<>(new HashMap<Integer, Object>());

        autoKeyedMap.put(0, TEST_VALUE);
    }

    /**
     * An automatically keyed map that uses the hash code of the value as it's key.
     *
     * @param <V> the value type for the map
     */
    private static final class HashCodeMap<V> extends AutoKeyedMap<Integer, V> {
        HashCodeMap(final Map<Integer, V> map, final boolean preventOverwrite) {
            super(map, preventOverwrite);
        }

        HashCodeMap(final Map<Integer, V> map) {
            super(map);
        }

        @Override
        public Integer getKey(final V value) {
            return value.hashCode();
        }
    }

    /**
     * A class that has a fake hash code (i.e. one that can be set at construction time).
     *
     * <p>
     * Note that this class breaks the {@code equals}/{@code hashCode} contract and should only be used for
     * testing.
     * </p>
     */
    private static final class FakeHashCodeObject {
        private final int hashCode;

        FakeHashCodeObject(final int hashCode) {
            this.hashCode = hashCode;
        }

        public int hashCode() {
            return hashCode;
        }
    }
}

I am primarily looking for feedback on my testing code, however feedback on the map implementation itself is also very welcome.

A few questions I have about my tests:

  1. Is it a good idea to just test the methods that have the "complex" logic? Am I testing everything I should here?

  2. How is my test documentation / commenting? Is it explicit enough (or even too explicit)? Does it tell you everything you need to know about the test?

  3. Is my TEST_VALUE constant alright as it is or should I directly inline the values?

    e.g this:

    // Add the test value to the map with a null key.
    autoKeyedMap.put(null, TEST_VALUE);
    
    // Get the generated key from the map.
    final Integer key = autoKeyedMap.getKey(TEST_VALUE);
    
    // Ensure that the key is correct.
    assertEquals(autoKeyedMap.get(key), TEST_VALUE);
    

    becomes this:

    // Add the test value to the map with a null key.
    autoKeyedMap.put(null, "Value");
    
    // Get the generated key from the map.
    final Integer key = autoKeyedMap.getKey("Value");
    
    // Ensure that the key is correct.
    assertEquals(autoKeyedMap.get(key), "Value");
    
\$\endgroup\$
0

1 Answer 1

4
\$\begingroup\$
public final Set<Map.Entry<K, V>> entrySet() {
    return map.entrySet();
}

From the documentation:

Returns a Set view of the mappings contained in this map. The set is backed by the map, so changes to the map are reflected in the set, and vice-versa. If the map is modified while an iteration over the set is in progress (except through the iterator's own remove operation, or through the setValue operation on a map entry returned by the iterator) the results of the iteration are undefined. The set supports element removal, which removes the corresponding mapping from the map, via the Iterator.remove, Set.remove, removeAll, retainAll and clear operations. It does not support the add or addAll operations.

Basically,

Map<String, Integer> map = new AutoKeyedMap<>(true);
map.put(null, 42);
for(Map.Entry<String, Integer> entry = map.entrySet()){
    entry.setValue(-1);
}

your map doesn't guarantee that values will not be overridden.


/**
 * Puts all of the key/values pairs from the provided map into the wrapped map.
 *
 * @param m the map to retrieve the key/value pairs from
 *
 * @see #put(Object, Object)
 * @see Map#putAll(Map)
 */
public final void putAll(final Map<? extends K, ? extends V> m) {
    for (final Map.Entry<? extends K, ? extends V> e : m.entrySet()) {
        put(e.getKey(), e.getValue());
    }
}

This is a method that, whilst it's good that you implemented it, really needs more thought.

You only allow non-null keys. Thus this method can only work if you only insert a map which has zero entries OR 1 entry with a null key.

You cannot make two AutoKeyedMaps and merge them via putAll. Consider put(null, e.getValue()).


@Test(expected = UnsupportedOperationException.class)
public void testPutNonNullKey() {
    final Map<Integer, Object> autoKeyedMap = new HashCodeMap<>(new HashMap<Integer, Object>());

    autoKeyedMap.put(0, TEST_VALUE);
}

This test is violating TDD and just ticking off checkboxes and code coverage. If a feature is unsupported, why does it have tests to ensure that it is not accidentally supported? Throw IllegalArgumentException instead; You don't allow non-null keys, so passing in a non-null argument is illegal.

 * <p>
 * If the key is not <code>null</code> then this method will throw an {@link UnsupportedOperationException}.
 *
 * This is due to the fact that the key is automatically derived by {@link #getKey(Object)} and should not be
 * provided manually.
 * </p>

You're violating Map's contract here: put's UnsupportedOperationException tends to mean "cannot put at all". From the documentation:

UnsupportedOperationException - if the put operation is not supported by this map

IllegalArgumentException - if some property of the specified key or value prevents it from being stored in this map


/**
 * A class that has a fake hash code (i.e. one that can be set at construction time).
 *
 * <p>
 * Note that this class breaks the {@code equals}/{@code hashCode} contract and should only be used for
 * testing.
 * </p>
 */
private static final class FakeHashCodeObject {
    private final int hashCode;

    FakeHashCodeObject(final int hashCode) {
        this.hashCode = hashCode;
    }

    public int hashCode() {
        return hashCode;
    }
}

Note that this class breaks the equals/hashCode contract and should only be used for testing.

No it doesn't.

HashCode docs:

Whenever it is invoked on the same object more than once during an execution of a Java application, the hashCode method must consistently return the same integer, provided no information used in equals comparisons on the object is modified. This integer need not remain consistent from one execution of an application to another execution of the same application.

Check. (internal variable is final)

If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

Check (doesn't override equals, so the only way to get a hit here is to be the same object).

It is not required that if two objects are unequal according to the equals(java.lang.Object) method, then calling the hashCode method on each of the two objects must produce distinct integer results. However, the programmer should be aware that producing distinct integer results for unequal objects may improve the performance of hash tables.

It fails this check, but that's not required.

As equals is not overridden, it can't violate that contract.

Do not lie in the comments, for it is worse than having no comments at all.


1: Is it a good idea to just test the methods that have the "complex" logic? Am I testing everything I should here?

Seems like you're doing fine, aside from the UnsupportedOperationException test.

2: How is my test documentation / commenting? Is it explicit enough (or even too explicit)? Does it tell you everything you need to know about the test?

I prefer to read test names; they're too short and don't tell me everything.

When I run my testing tool, I see the method names in the list as "test passed" or "test failed". It is INCREDIBLY handy to be able to spot that every method with "preventOverwrite" in the name has failed. Your names don't quite allow this. The name of a test method can be longer than usual; nobody has to actually call it by name. testPutPreventOverwrite() becomes testPutWithPreventOverwriteOnEmptyMap() for me. Maybe the "With" is optional.

3: Is my TEST_VALUE constant alright as it is or should I directly inline the values?

Wait, you only have one?

... You're missing the test cases where you add multiple values to the map and see if it actually can store multiple values.

But no, don't inline. That'd be annoying.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the great answer, one thing I've been trying to work out is how to handle your first point; an Entry can't change it's own key, meaning that setValue can't work. I could throw an UnsupportedOperationException (although the documentation for Map#setValue(V) states that an UnsupportedOperationException means the map doesn't support put, which is incorrect for my map implementation. [1/2] \$\endgroup\$ Jun 30, 2016 at 4:48
  • \$\begingroup\$ I can't remove the corresponding Entry object and re-add it using put either, as this then "invalidates" the entry (some Entry implementations can throw an IllegalStateException if they're used after being removed from a map) and it might cause some strange things to happen during iteration (although this depends on the map being wrapped). This might work but I'm worried about any impacts it may have further down the line. [2/2] \$\endgroup\$ Jun 30, 2016 at 4:48
  • 1
    \$\begingroup\$ @JackWilsdon that's because you're abusing the Map class beyond its initial contract already. The map is not supposed to generate keys, the user of the map does. For instance, take the get method. A key goes in, a value comes out. Take the put method. A key and value go in, the previous value comes out. For your map, this makes no sense! For your map, I'd expect put to take a value, and return a key - and then use get as normal. \$\endgroup\$
    – Pimgd
    Jun 30, 2016 at 7:34
  • \$\begingroup\$ It sounds like it may be worth creating my own Map-style interface then :/ thanks! \$\endgroup\$ Jun 30, 2016 at 12:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.