HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Find items that appear the same number of times in two collections

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
samethenumbertwoitemsthatcollectionsfindtimesappear

Problem

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:

Appearance class:

package assign1;

import java.util.*;

public class Appearances {

    public static  int sameCount(Collection a, Collection b) {

        Map map1 = countItemsInCollection(a);
        Map map2 = countItemsInCollection(b);

        return compareMapForMatchingKeyAndValue(map1, map2);
    }

    private static  Map countItemsInCollection(Collection collection) {

        Map 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  int compareMapForMatchingKeyAndValue(Map map1, Map map2) {
        Set> set1 = map1.entrySet();
        Set> set2 = map2.entrySet();

        Set> 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 stringToList(String s) {
List list = new ArrayList();
for (int i = 0; i a = stringToList("abbccc");
List b = stringToList("cccbba");
assertEquals(3, Appearances.sameCount(a, b));
}

@Test
public void testSameCount2() {
// basic List cases
List a = Arrays.asList(1, 2, 3, 1, 2, 3,

Solution

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, it would be slightly simpler to use a List.

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

Context

StackExchange Code Review Q#132483, answer score: 5

Revisions (0)

No revisions yet.