6
\$\begingroup\$

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<String, Integer> findOccurences(String str) {
        Map<String, Integer> map = new HashMap<String, Integer>();
        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    <<< includes space 
s T 1
his 2
Thi 2
is  1    <<< includes space
\$\endgroup\$
0
8
\$\begingroup\$

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<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;
}

And it would be good to add a unit test for it:

@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));
}
\$\endgroup\$

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.