# A reliable way to remove consecutive appearances of a substring

I'm attempting to write a piece of code that is supposed to remove consecutive appearances of a string (not a single character) in a StringBuilder.
It's extremely important that the method works well and does not remove or change anything that it shouldn't. Solid performance is a secondary requirement.

for example:

Input: "xxxxABCxxxxABCABCxxxxABABCxxABCABCABC"
Remove consecutive: "ABC"

Output: "xxxxABCxxxxABCxxxxABABCxxABC"


I've written a method that does this and testing shows that it works as expected, but it will be catastrophic if I use it and it results in changing the StringBuilder in an unexpected way - that's why I want another opinion on whether my code is really 'safe' in all cases:

public static void RemoveConsecutive(this StringBuilder sb, string value)
{
if (sb == null)
throw new ArgumentNullException("sb");
if (value == null)
throw new ArgumentNullException("value");
if (value == string.Empty)
throw new ArgumentException("value cannot be an empty string.", "value");

bool justRemoved = false;
for (int i = 0; i < sb.Length - value.Length; i++)
{
if (justRemoved || Util.ExistsAt(sb, i, value))
{
if (Util.ExistsAt(sb, i + value.Length, value))
{
sb.Remove(i, value.Length);
justRemoved = true;
i--;
}
else
{
justRemoved = false;
}
}
}
}

// Checks if the provided string appears in the StringBuilder at the specified index
public static bool ExistsAt(StringBuilder sb, int startIndex, string str)
{
if (startIndex < 0 || startIndex >= sb.Length)
throw new ArgumentOutOfRangeException("startIndex", "startIndex must be a valid index in the provided StringBuilder.");
if (startIndex + str.Length > sb.Length)
return false;
for (int i = 0; i < str.Length; i++)
{
if (str[i] != sb[i + startIndex])
return false;
}
return true;
}


While this piece of code does something rather trivial, I just want another set of eyes to look at it and possibly find faults I could not. Again, any sort of unexpected behavior might be catastrophic.

-
If it's so important, have you written unit tests for it? – svick Feb 10 '13 at 2:22
@svick Yes. I have tested it rather extensively, but what I'm scared of is some sort of 'edge case' that I might not come across in my tests. – Acidic Feb 10 '13 at 3:39
You state the problem as "remov[ing] consecutive appearances of a substring" and give an example of "xxxxABCxxxxABCABCxxxxABABCxxABCABCABC" -> "xxxxABCxxxxABCxxxxABABCxxABC". But (replacing your xs with digits for visibility) shouldn't that result be 0123ABC45678901ABABC23? – Ross Patterson Feb 10 '13 at 13:47
@RossPatterson The additional instances of ABC you removed were not consecutive. – svick Feb 10 '13 at 13:57
@svick I beg to differ. "ABC" -> "ABC", "ABCABC" -> "", "ABABC" -> "ABABC", "ABCABCABC" -> "". The second and fourth cases have consecutive ABCs. The example suggests eliminating groups of doubled ABCs, which is not the same thing. Hence my question. – Ross Patterson Feb 10 '13 at 14:31

Your code looks like it does exactly what you want to me.

Some notes:

1. StringBuilder methods (which you're emulating by using an extension method) return the modified StringBuilder, so they can be used in a fluent manner. You should probably do the same.
2. I don't like changing the index of a for loop. Especially since i-- means “stay at the same index”, I think that's confusing. You should use a while loop instead.
3. You can shorten your code by quite a bit by using Regex:

public static string RemoveConsecutive(string text, string value)
{
// validation omited

string regex = string.Format(@"({0})+", Regex.Escape(value));
return Regex.Replace(text, regex, match => value);
}


Shorter code is usually less error prone. But in this case, I'm not so sure about that, because there are some intricacies in this code:

1. Calling Escape() is crucial if value contains some regex special characters (e.g. *).
2. If the value was used directly as the replacement (instead of a delegate), it wouldn't work correctly if it contained any substitution (e.g. \$0).
-
Indeed, this would seem to be one of those occasions when using regular expressions does not give you two problems. – Ross Patterson Feb 10 '13 at 13:55