Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
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

1 Answer 1

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).
share|improve this answer
2  
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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.