patternjavaMinor
Find items that appear the same number of times in two collections
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
Problem:
Appearance class:
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,
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:
All methods should have names that describe their purpose. In this particular example, it would be great to add test methods for these cases:
Btw I would use these statements as test method names, with spaces replaced with underscores.
Simplify
In the tests, instead of a
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
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.