4
\$\begingroup\$

Here is a Java collection puzzle I found online. I was wondering if anyone can provide me with suggestions on how Appearance and tests can be improved. Suggestions of any kind and scope are very welcomed. it would be even better if reasons can be provided along with the suggestions.

Problem:

enter image description here

Appearance class:

package assign1;

import java.util.*;

public class Appearances {


    public static <T> int sameCount(Collection<T> a, Collection<T> b) {

        Map<T, Integer> map1 = countItemsInCollection(a);
        Map<T, Integer> map2 = countItemsInCollection(b);

        return compareMapForMatchingKeyAndValue(map1, map2);
    }

    private static <T> Map<T, Integer> countItemsInCollection(Collection<T> collection) {

        Map<T, Integer> mapItemToNumberOfOccurance = new HashMap<>();

        for (T item : collection) {
            if (!mapItemToNumberOfOccurance.containsKey(item)) {
                mapItemToNumberOfOccurance.put(item, 1);
            } else {
                mapItemToNumberOfOccurance.put(item, mapItemToNumberOfOccurance.get(item) + 1);
            }
        }

        return mapItemToNumberOfOccurance;
    }

    private static <T> int compareMapForMatchingKeyAndValue(Map<T, Integer> map1, Map<T, Integer> map2) {
        Set<Map.Entry<T, Integer>> set1 = map1.entrySet();
        Set<Map.Entry<T, Integer>> set2 = map2.entrySet();

        Set<Map.Entry<T, Integer>> matchingSet = new HashSet<>(set1);

        matchingSet.retainAll(set2);

        return matchingSet.size();

    }

}

Test class:

package assign1;

import assign1.Appearances;
import static org.junit.Assert.*;
import org.junit.Test;

import java.util.*;

public class AppearancesTest {
    // utility -- converts a string to a list with one
    // elem for each char.

    private List<String> stringToList(String s) {
        List<String> list = new ArrayList<String>();
        for (int i = 0; i < s.length(); i++) {
            list.add(String.valueOf(s.charAt(i)));
            // note: String.valueOf() converts lots of things to string form
        }
        return list;
    }

    @Test
    public void testSameCount1() {
        List<String> a = stringToList("abbccc");
        List<String> b = stringToList("cccbba");
        assertEquals(3, Appearances.sameCount(a, b));
    }

    @Test
    public void testSameCount2() {
        // basic List<Integer> cases
        List<Integer> a = Arrays.asList(1, 2, 3, 1, 2, 3, 5);
        assertEquals(1, Appearances.sameCount(a, Arrays.asList(1, 9, 9, 1)));
        assertEquals(2, Appearances.sameCount(a, Arrays.asList(1, 3, 3, 1)));
        assertEquals(1, Appearances.sameCount(a, Arrays.asList(1, 3, 3, 1, 1)));
    }

    @Test
    public void testSameCount3(){
        List<Double> a = Arrays.asList(2.2, 3.3, 4.4, 1.0, 4.4, 2.2, 3.3);
        assertEquals(1, Appearances.sameCount(a, Arrays.asList(2.2, 2.2, 3.3)));
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you on Java 8? \$\endgroup\$ – h.j.k. Jun 22 '16 at 15:07
  • 1
    \$\begingroup\$ @h.j.k. yes I am \$\endgroup\$ – Thor Jun 23 '16 at 0:49
5
\$\begingroup\$

Nicely done, an efficient solution to the problem, easy to read and unit tested. For the sake of completeness, it would be good to include time and space complexity analysis. My main objections are concerning the unit testing technique and naming.

Unit testing

Each test method should have a single, clear purpose. The test methods here essentially pick 3 example inputs, but it's not clear how you picked those examples.

In general, tests should aim to cover cases like:

  • Happy paths: simple execution paths that run with success
  • Interesting corner cases
  • Invalid paths: invalid, tricky, malicious inputs

All methods should have names that describe their purpose. In this particular example, it would be great to add test methods for these cases:

  • Should get 0 when the collections have nothing in common
  • Should get size if the collections have identical content
  • Should get 0 if either ecollection is empty
  • Should get size of collection if the other is superset

Btw I would use these statements as test method names, with spaces replaced with underscores.

Simplify

In the tests, instead of a List<String>, it would be slightly simpler to use a List<Character>.

Naming

Some of the names are a bit overcomplicated. I suggest some simplifications:

  • countItemsInCollection: as collection is already implied by the parameter type, I would drop that word from the name

  • compareMapForMatchingKeyAndValue: a method that starts with the name compare gives the impression that it might follow the contract of comparators, returning either of -1, 0, or 1. So it's a bit misleading. Since this method effectively calculates the intersection of map entries, I would call it intersectEntries, or even just simply intersect

  • mapItemToNumberOfOccurance: how about simply counts, or itemCounts

\$\endgroup\$
  • \$\begingroup\$ thank you so much for the detailed explanation. really appreciate your help. However, I was just wondering if there anyway in java to test a class without declaring it public due to the reason of encapsulation? \$\endgroup\$ – Thor Jun 21 '16 at 2:12
  • 1
    \$\begingroup\$ A class with protected or package local visibility can be tested if you put the unit tests in the same package. If you want to make a class private, then I recommend to test it through other classes that are public. That is, test the public API only. There are ways to test private parts of the code, using PowerMock, but I don't recommend it, due to violating encapsulation. \$\endgroup\$ – janos Jun 21 '16 at 4:47
1
\$\begingroup\$

Java 8 stream-based processing

Since you are on Java 8, you can consider replacing your countItemsInCollection(Collection) implementation with a stream-based approach:

private static <T> Map<T, Integer> countItemsInCollection(Collection<T> collection) {
    return collection.stream().collect(Collectors.groupingBy(
                                        Function.identity(), Collectors.counting()));
}
  1. stream() on your collection,
  2. groupingBy() each element using its identity() as the key and counting() its occurrences as the value.
\$\endgroup\$
  • 1
    \$\begingroup\$ @hjk thank you very much for the suggestion! will definitly put java 8 and lambda to use in the future! \$\endgroup\$ – Thor Jun 25 '16 at 2:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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