Replacing multiple search strings simultaneously

Question

A utility method StringUtils.replace to replace multiple search strings simultaneously. I'm looking for a review of any and all aspects, but especially:

• Is there's a better way to do this? (short of external libraries)
• Is the parameter validation OK the way I did?
• Is the way I test adequate and up to good practices in testing?

The code:

import java.util.*;
import java.util.stream.Stream;

public class StringUtils {

    public static final String ERR_NULL_PARAM = "none of the parameters should be null";
    public static final String ERR_SEARCHSTRINGS_REPLACEMENTS_LENGTH_MISMATCH =
            "there must be the same number of search strings and replacements";
    public static final String ERR_NULL_OR_EMPTY_SEARCHSTRING = "there must be no null element or empty search string";
    public static final String ERR_NULL_REPLACEMENT = "there must be no null element in replacements";
    public static final String ERR_DUPLICATE_SEARCHSTRINGS = "search strings must be distinct";

    /**
     * Replace multiple search strings simultaneously
     *
     * @param text the source text
     * @param searchStrings search strings to replace
     * @param replacements texts to replace the corresponding search strings
     * @return new text with search strings replaced
     */
    public static String replace(String text, String[] searchStrings, String[] replacements) {
        validateParams(text, searchStrings, replacements);

        StringBuffer buffer = new StringBuffer();
        Pattern pattern = Pattern.compile(Stream.of(searchStrings).collect(joining("|")));
        Matcher matcher = pattern.matcher(text);
        while (matcher.find()) {
            String match = matcher.group();
            for (int i = 0; i < searchStrings.length; ++i) {
                if (match.equals(searchStrings[i])) {
                    matcher.appendReplacement(buffer, replacements[i]);
                    break;
                }
            }
        }
        matcher.appendTail(buffer);
        return buffer.toString();
    }

    private static void validateParams(String text, String[] searchStrings, String[] replacements) {
        if (text == null || searchStrings == null || replacements == null) {
            throw new IllegalArgumentException(ERR_NULL_PARAM);
        }
        if (searchStrings.length != replacements.length) {
            throw new IllegalArgumentException(ERR_SEARCHSTRINGS_REPLACEMENTS_LENGTH_MISMATCH);
        }
        if (searchStrings.length == 0) {
            return;
        }
        if (anyNullOrEmpty(searchStrings)) {
            throw new IllegalArgumentException(ERR_NULL_OR_EMPTY_SEARCHSTRING);
        }
        if (anyNull(replacements)) {
            throw new IllegalArgumentException(ERR_NULL_REPLACEMENT);
        }
        if (containsDuplicates(searchStrings)) {
            throw new IllegalArgumentException(ERR_DUPLICATE_SEARCHSTRINGS);
        }
    }

    private static boolean anyNullOrEmpty(String[] strings) {
        return Stream.of(strings).allMatch(x -> x == null || x.isEmpty());
    }

    private static boolean anyNull(String[] strings) {
        return Stream.of(strings).allMatch(x -> x == null);
    }

    private static boolean containsDuplicates(String[] strings) {
        return Stream.of(strings).distinct().count() != strings.length;
    }
}

Unit tests:

import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;

import static com.janosgyerik.tools_wip.StringUtils.*;
import static org.junit.Assert.assertEquals;

public class StringUtilsTest {
    @Rule
    public ExpectedException thrown = ExpectedException.none();

    @Test
    public void test_empty_text() {
        assertEquals("", replace("", new String[]{"foo"}, new String[]{"bar"}));
    }

    @Test
    public void test_empty_searchstrings_and_replacements() {
        assertEquals("", replace("", new String[0], new String[0]));
    }

    @Test
    public void test_replace_one_searchstring_once() {
        String before = "foo";
        String after = "bar";
        assertEquals(after, replace(before, new String[]{before}, new String[]{after}));
    }

    @Test
    public void test_replace_one_searchstring_twice() {
        String before = "foo";
        String after = "bar";
        assertEquals(after + after, replace(before + before, new String[]{before}, new String[]{after}));
    }

    @Test
    public void test_replace_two_searchstrings_simultaneously() {
        String before1 = "foo";
        String after1 = "bar";
        String after2 = "baz";
        assertEquals(after1 + after2, replace(before1 + after1,
                new String[]{before1, after1},
                new String[]{after1, after2}));
    }

    @Test
    public void test_no_matches() {
        String text = "foofoo";
        assertEquals(text, replace(text, new String[]{"notmatching"}, new String[]{"bar"}));
    }

    @Test
    public void test_source_overlap() {
        assertEquals("foovel", replace("applevel", new String[]{"apple", "level"}, new String[]{"foo", "bar"}));
    }

    @Test
    public void test_result_overlap() {
        assertEquals("spacemarvel", replace("applevel",
                new String[]{"apple", "marvel"},
                new String[]{"spacemar", "bar"}));
    }

    @Test
    public void test_replace_multiple_simultaneous_searchstrings() {
        assertEquals("Once upon a foo, there was a bar and a baz, and another bar and a cat.",
                replace("Once upon a baz, there was a foo and a bar, and another foo and a cat.",
                        new String[]{"foo", "bar", "baz"},
                        new String[]{"bar", "baz", "foo"})
        );
    }

    @Test
    public void test_circular_replacement() {
        assertEquals("barfoo", replace("foobar", new String[]{"foo", "bar"}, new String[]{"bar", "foo"}));
    }

    @Test
    public void test_null_text_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_NULL_PARAM);

        replace(null, new String[0], new String[0]);
    }

    @Test
    public void test_null_searchstrings_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_NULL_PARAM);

        replace("", null, new String[0]);
    }

    @Test
    public void test_null_replacements_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_NULL_PARAM);

        replace("", new String[0], null);
    }

    @Test
    public void test_more_replacements_than_searchstrings_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_SEARCHSTRINGS_REPLACEMENTS_LENGTH_MISMATCH);

        replace("", new String[0], new String[]{"bar"});
    }

    @Test
    public void test_more_searchstrings_than_replacements_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_SEARCHSTRINGS_REPLACEMENTS_LENGTH_MISMATCH);

        replace("", new String[]{"foo"}, new String[0]);
    }

    @Test
    public void test_null_element_in_searchstrings_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_NULL_OR_EMPTY_SEARCHSTRING);

        replace("", new String[]{null}, new String[]{"bar"});
    }

    @Test
    public void test_empty_element_in_searchstrings_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_NULL_OR_EMPTY_SEARCHSTRING);

        replace("", new String[]{""}, new String[]{"bar"});
    }

    @Test
    public void test_null_element_in_replacements_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_NULL_REPLACEMENT);

        replace("", new String[]{"foo"}, new String[]{null});
    }

    @Test
    public void test_non_distinct_searchstrings_should_throw() {
        thrown.expect(IllegalArgumentException.class);
        thrown.expectMessage(ERR_DUPLICATE_SEARCHSTRINGS);

        String before = "foo";
        replace("", new String[]{before, before}, new String[]{"bar", "baz"});
    }
}

Answer 1

This is a StringUtils class, not RegexUtils. Therefore, I would expect all of the searchStrings to be taken literally. You must quote each of the searchStrings when composing your regex.

For figuring out which replacement string goes with which search string, it might be better to use a HashMap<String, String> instead of a linear search.

Answer 2

There are few small things that stick out to me... starting with the least significant, and moving up...

Constants

Why are your error message text values public? If it is to satisfy the Unit testing, then the common solution is to make them package-private, and put the unit tests in the same package as the code. Most common build tools (ant, maven, etc.) have separate folders for the tests and the source, and you can have the same package structures in each.

If you really want to test the actual exception text, then the better solution would be to have separate specific exception types for each problem type, or to reuse different existing exceptions. For example - it is quite OK to throw a NullPointerException if one of the input strings are null when they should not be. An IllegalArgumentException for empty-strings is OK. Or, create the DuplicateSearchStringException if you need it.

any vs. not-all

In your validation streams, you have code like:

private static boolean anyNullOrEmpty(String[] strings) {
    return Stream.of(strings).allMatch(x -> x == null || x.isEmpty());
}

That would be better as:

private static boolean anyNullOrEmpty(String[] strings) {
    return !Stream.of(strings)
          .filter(x -> x == null || x.isEmpty())
          .findAny()
          .isEmpty();
}

It's better because it stops streaming when the first problem is found - it does not need to check everything.

String joining

Stream.of(searchStrings).collect(joining("|")

can be simplified as:

String.join("|", searchStrings)

Having said that, you should also consider your search-patterns - are you looking for exact strings? You should probably be escaping the inputs, if you are. Otherwise, what if someone inputs "Hi|there" now what?

Stream.of(searchStrings).map(Pattern::quote).collect(joining("|")

Answer 3

These test cases were not strict enough and have lead to overlooked bugs:

@Test
public void test_null_element_in_searchstrings_should_throw() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage(ERR_NULL_OR_EMPTY_SEARCHSTRING);

    replace("", new String[]{null}, new String[]{"bar"});
}

@Test
public void test_empty_element_in_searchstrings_should_throw() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage(ERR_NULL_OR_EMPTY_SEARCHSTRING);

    replace("", new String[]{""}, new String[]{"bar"});
}

@Test
public void test_null_element_in_replacements_should_throw() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage(ERR_NULL_REPLACEMENT);

    replace("", new String[]{"foo"}, new String[]{null});
}

These test null or empty values in the search strings, or null values in the replacements should throw validation errors. However, as test subjects, they use:

• new String[]{null} as search strings : all values are null
• new String[]{""} as search strings : all values are empty
• new String[]{null} as replacements : all values are null

As it happens the implementation has bugs that these tests fail to check properly:

private static boolean anyNullOrEmpty(String[] strings) {
    return Stream.of(strings).allMatch(x -> x == null || x.isEmpty());
}

private static boolean anyNull(String[] strings) {
    return Stream.of(strings).allMatch(x -> x == null);
}

Despite its name, anyNullOrEmpty returns true if all values are null or empty. As a result, validation will only raise an error when all values are null or empty. If there is one non-null non-empty element, validation would incorrectly pass without raising errors.

The first step of the fix should be adjusting the test cases to expose the bugs:

@Test
public void test_null_element_in_searchstrings_should_throw() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage(ERR_NULL_OR_EMPTY_SEARCHSTRING);

    replace("", new String[]{"foo", null}, new String[]{"bar", "baz"});
}

@Test
public void test_empty_element_in_searchstrings_should_throw() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage(ERR_NULL_OR_EMPTY_SEARCHSTRING);

    replace("", new String[]{"foo", ""}, new String[]{"bar", "baz"});
}

@Test
public void test_null_element_in_replacements_should_throw() {
    thrown.expect(IllegalArgumentException.class);
    thrown.expectMessage(ERR_NULL_REPLACEMENT);

    replace("", new String[]{"foo", "bar"}, new String[]{"baz", null});
}

And the fix is to simply replace allMatch with anyMatch:

private static boolean anyNullOrEmpty(String[] strings) {
    return Stream.of(strings).anyMatch(x -> x == null || x.isEmpty());
}

private static boolean anyNull(String[] strings) {
    return Stream.of(strings).anyMatch(x -> x == null);
}