The usual changes but:
- The “FirstRepeatedLetter.findIn()” has been improved out of the box. In particular returning ‘\0’ is too C-like and smells like “primitive obsession”. We have
null
, we should use it. What if the input contains\0
as the matching character? - A slightly better code (IMHO) is in
FirstRepeatedLetterBetter.findIn()
- The final refactored code returns not null, but
Optional<Character>
, which is cleaner. - Again, for testing we use a common interface or a wrapper class to avoid having to duplicate testing code.
package chapter11; import org.junit.jupiter.api.Test; import java.util.Optional; import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.*; public class ReworkTheLogicTest { interface FindLetter { Character findIn(String word); } static class FirstRepeatedLetterBefore implements FindLetter { public Character findIn(final String word) { final char[] letters = word.toCharArray(); for (char candidate : letters) { int count = 0; for (char letter : letters) { if (candidate == letter) { count++; } } if (count > 1) { return candidate; } } return null; } } static class FirstRepeatedLetterBetter implements FindLetter { public Character findIn(final String word) { char[] letters = word.toCharArray(); int i = 0; Character found = null; while (i < letters.length && found == null) { // will letter[i] been seen again? char maybe = letters[i]; int j = i + 1; while (j < letters.length && letters[j] != maybe) j++; if (j < letters.length) { found = maybe; } else { i++; } } return found; } } // Does NOT implement "FindLetter", returns Optional<Character> static class FirstRepeatedLetterAfter { public Optional<Character> findIn(final String word) { return Stream.of(word.split("")) .filter(letter -> word.lastIndexOf(letter) > word.indexOf(letter)) .findFirst() .map(letter -> letter.charAt(0)); } } static class FirstRepeatedLetterAfterWrapped implements FindLetter { private final FirstRepeatedLetterAfter finder; public FirstRepeatedLetterAfterWrapped(FirstRepeatedLetterAfter finder) { this.finder = finder; } public Character findIn(final String word) { return finder.findIn(word).orElse(null); } } private static void commonFindFirstRepeatingTests(final FindLetter finder) { assertAll( () -> assertEquals('l', finder.findIn("hello")), () -> assertEquals('h', finder.findIn("hellothere")), () -> assertEquals('a', finder.findIn("magicalguru")), () -> assertEquals('z', finder.findIn("abcdefghijklmnopqrstuvwxyzz")), () -> assertNull(finder.findIn("once")), () -> assertNull(finder.findIn("")) ); } @Test void findFirstRepeatingBefore() { commonFindFirstRepeatingTests(new FirstRepeatedLetterBefore()); } @Test void findFirstRepeatingBetter() { commonFindFirstRepeatingTests(new FirstRepeatedLetterBetter()); } @Test void findFirstRepeatingAfter() { commonFindFirstRepeatingTests(new FirstRepeatedLetterAfterWrapped(new FirstRepeatedLetterAfter())); } }