patternjavaMinor
Find repeated substrings of length 3 in a string
Viewed 0 times
substringsrepeatedlengthfindstring
Problem
The following method receives a string and finds all occurrences of repeated substrings with a length of three characters, including spaces. Is this a good approach?
Input
Output
` Th 1
public Map findOccurences(String str) {
Map map = new HashMap();
int counter = 0;
String sub;
int stringLen = str.length() - 2;
for (int i = 0; i < stringLen ; i++) {
sub = str.substring(i, i + 3);
if (map.containsKey(sub)) {
counter = map.get(sub);
counter++;
map.put(sub, counter);
counter = 0;
} else {
map.put(sub, 1);
}
}
return map;
}Input
This This
Output
` Th 1
Solution
It's quite fine. The logic is simple and clear.
There might be a more clever solution (I don't know),
but probably it will be less clear.
A few improvements would be good though:
Based on the above suggestions, the implementation could become a bit simpler:
And it would be good to add a unit test for it:
There might be a more clever solution (I don't know),
but probably it will be less clear.
A few improvements would be good though:
counteris set to 0 at two places, both unnecessary, as the value is overwritten later
- It's a good practice to declare variables in the smallest scope where they are used. For example
counterandsubwould be better to declare inside the loop.
stringLenis not a great name, because it's not really the "length" of anything. Evenlimitwould be better
- Instead of the magic number 3, and
str.length() - 2, it would be better to make the target length a method parameter.
- Instead of calling
.containsKeyfollowed bygetandput, I prefer toget, checknulland thenput, whenever possible. This is not always possible, for example whennullis a meaningful entry, but that's not the case here.
Based on the above suggestions, the implementation could become a bit simpler:
public Map findOccurences(String str, int length) {
Map map = new HashMap<>();
int limit = str.length() - length + 1;
for (int i = 0; i < limit; i++) {
String sub = str.substring(i, i + length);
Integer counter = map.get(sub);
if (counter == null) {
counter = 0;
}
map.put(sub, ++counter);
}
return map;
}And it would be good to add a unit test for it:
@Test
public void test_This_This_() {
Map map = new HashMap<>();
map.put("his", 2);
map.put("Thi", 2);
map.put(" Th", 1);
map.put("s T", 1);
map.put("is ", 2);
assertEquals(map, findOccurences("This This ", 3));
}Code Snippets
public Map<String, Integer> findOccurences(String str, int length) {
Map<String, Integer> map = new HashMap<>();
int limit = str.length() - length + 1;
for (int i = 0; i < limit; i++) {
String sub = str.substring(i, i + length);
Integer counter = map.get(sub);
if (counter == null) {
counter = 0;
}
map.put(sub, ++counter);
}
return map;
}@Test
public void test_This_This_() {
Map<String, Integer> map = new HashMap<>();
map.put("his", 2);
map.put("Thi", 2);
map.put(" Th", 1);
map.put("s T", 1);
map.put("is ", 2);
assertEquals(map, findOccurences("This This ", 3));
}Context
StackExchange Code Review Q#64952, answer score: 8
Revisions (0)
No revisions yet.