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

Find repeated substrings of length 3 in a string

Submitted by: @import:stackexchange-codereview··
0
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?

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:

  • counter is 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 counter and sub would be better to declare inside the loop.



  • stringLen is not a great name, because it's not really the "length" of anything. Even limit would 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 .containsKey followed by get and put, I prefer to get, check null and then put, whenever possible. This is not always possible, for example when null is 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.