7
\$\begingroup\$

I want to check a text for consecutive repeating characters. For example, with a minimum of 3 consecutive chars, this should match:

  • Okeee
  • OOOkee
  • Alsoook

This should not match:

  • No not okee, oh no

This is my code:

public static bool HasConsecutiveChars(string source, int sequenceLength)
        {
            if (string.IsNullOrEmpty(source))
            {
                return false;
            }

            if (source.Length == 1)
            {
                return false;
            }

            int charCount = 1;

            for (int i = 0; i < source.Length - 1; i++)
            {
                char c = source[i];
                if (c == source[i + 1])
                {
                    charCount++;

                    if (charCount >= sequenceLength)
                    {
                        return true;
                    }
                }
                else
                {
                    charCount = 1;
                }
            }

            return false;
        }

It works but I feel there should be a cleaner solution to this problem.

\$\endgroup\$
14
\$\begingroup\$

Edit - there is a bug with this: see my other answer

This is very easy with Regular Expressions:

public static bool HasConsecutiveChars(string source, int sequenceLength)
{
    // should check sequence length

    // just repeating letters
    return Regex.IsMatch(source,"([a-zA-Z])\\1{"+ (sequenceLength - 1) + "}");

    // any character version
    // return Regex.IsMatch(source,"(.)\\1{"+ (sequenceLength - 1) + "}");
}

The regular expression simply captures each letter into a group and then checks if it is repeated the number of times minus one. I've omitted range checking.

Talking about your code for a moment:

Great method name - would be perfect as an extension method in my opinion.

These two checks can be merged:

if (string.IsNullOrEmpty(source))
{
    return false;
}

if (source.Length == 1)
{
    return false;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ OP does not specify that the character must be alphabetic...I would think (.)\1{2,} would be the appropriate expression for a requested sequence length of 3, for example. \$\endgroup\$ – Adrian Larson Sep 2 '15 at 18:32
  • \$\begingroup\$ @AdrianLarson - I limited it to letters as that was what all the examples used. \$\endgroup\$ – RobH Sep 2 '15 at 19:22
  • 1
    \$\begingroup\$ I would still argue it does not meet the requirements as stated. \$\endgroup\$ – Adrian Larson Sep 2 '15 at 19:24
  • \$\begingroup\$ @AdrianLarson - edited in the alternate Regex. Although my answer matches the test cases provided it does obviously deviate when a string contains a number of repeated non letter characters. \$\endgroup\$ – RobH Sep 2 '15 at 19:30
10
\$\begingroup\$

Although both given answers are pretty good, one using Regex and the other using a different approach, neither of these answers pointed out the following flaw

  • if the passed in int sequenceLength is 1 a source.Length == 1 should just return true.

Some minor things

  • a passed in negative sequenceLength should throw an ArgumentOutOfRangeException

  • a passed in source which is null should throw an ArgumentNullException

  • if source.Length < sequenceLength the method should return early.

  • initializing int charCount = 0; together with the changed if condition like so

    if (charCount == sequenceLength)
    

    would make it more clear what the purpose is.

\$\endgroup\$
  • \$\begingroup\$ Excellent points there - I skirted over it all with "should check sequence length" \$\endgroup\$ – RobH Sep 2 '15 at 8:50
4
\$\begingroup\$

Forgive the second answer - it's very separate to my other answer so I think it warrants a new one.

A bug?

Your code (and my suggested solution too) has a bug - UTF16 surrogate pairs...

Here's an example:

// Unicode character 🀜
var fourCircles = char.ConvertFromUtf32(0x1F01C);

// string 🀜🀜🀜 (same character repeated 3 times)
var twelveCircles = fourCircles + fourCircles + fourCircles;

HasConsecutiveChars(twelveCircles, 3); // False

Each char instance is one code point - a surrogate pair is made up of two encoding values in special ranges. So although we would consider it one 'character' it is in fact 2 char instances in C#.

See here for a better explanation than I could ever put together.

The Unicode Standard defines a surrogate pair as a coded character representation for a single abstract character that consists of a sequence of two code units. The first value of the surrogate pair is the high surrogate, a 16-bit code value in the range of U+D800 through U+DBFF. The second value of the pair is the low surrogate, in the range of U+DC00 through U+DFFF.

Another slightly less weird bug is the following string:

 "ééé"

or

 "e\u0301e\u0301e\u0301"

Both of those strings also result in False even though they should be True IMO.

Strings are really hard - these bugs probably won't matter for you but it's always worth pointing out really obscure edge cases ;)

I should point out that I'm on my mac so I had to do my testing on ideone (mono) but results should be the same on windows too.

I couldn't resist trying to make a solution that works for all chars...

// using System;
// using System.Globalization;
// using System.Text;

public static bool HasConsecutiveChars(string source, int sequenceLength)
{
    // omitted argument checking (see Heslacher's answer)
    var charEnumerator = StringInfo.GetTextElementEnumerator(source);
    var currentElement = string.Empty;
    int count = 1;
    while (charEnumerator.MoveNext()) 
    {
        if (currentElement == charEnumerator.GetTextElement())
        {
             if (++count >= sequenceLength)
             {
                return true;
             }
        }
        else
        {
            count = 1;
            currentElement = charEnumerator.GetTextElement();
        }
    }
    return false;
}

Here's it working on ideone

\$\endgroup\$
  • \$\begingroup\$ The issue isn't that strings are hard from a computer science perspective: their behavior in automata is well understood. The issue is mapping real world data streams to alphabets without defining the alphabets beforehand...or more practically speaking the issue is things going wrong when we do that. Or to put it another way, the issue is not asserting our assumptions about the data stream in our code and treating some data as invalid based on our assumptions (e.g. non-ASCII characters). is a letter. We may encode/decode it as character. Awareness that we are doing so is paramount. \$\endgroup\$ – ben rudgers Sep 3 '15 at 14:07
  • \$\begingroup\$ @benrudgers - exactly so. I meant that statement in a .Net specific way. My main problem is the word "character" (and therefore char) it can mean too many different things. Generally looping through chars in a string is going to introduce unicode edge cases! \$\endgroup\$ – RobH Sep 3 '15 at 14:27
2
\$\begingroup\$
  1. These two ifs need not be nested:

    if (c == source[i + 1])
    {
        charCount++;
    
        if (charCount >= sequenceLength)
        {
    
  2. Instead of using a for loop, you could use a foreach instead. Save the first character in a lastSeen variable, then iterate over the remaining characters (source.Skip(1))

Refactored

public static bool HasConsecutiveChars(string source, int sequenceLength)
{

    if(string.IsNullOrEmpty(source) || source.Length == 1)
        return false;

    char lastSeen = source.First();
    var count = 1;

    foreach(var c in source.Skip(1))
    {
        if (lastSeen == c)
            count++;
        else
            count = 1;

        if (count == sequenceLength)
            return true;

        lastSeen = c;
    }

    return false;
}
\$\endgroup\$
  • \$\begingroup\$ Why re-invent the wheel when a regex check will do the same thing? \$\endgroup\$ – David Arno Sep 2 '15 at 12:10
  • \$\begingroup\$ Not everyone likes to use regex. On another note you could use count = lastSeen == c ? count + 1 : 1; looks a bit cleaner imo, but that's a matter of preference I suppose. \$\endgroup\$ – Shelby115 Sep 2 '15 at 13:04
  • 1
    \$\begingroup\$ @DavidArno I actually agree that regex would be a good option here (although, as Shelby pointed out, not everyone likes regex). But the point of my answer wasn't that you shouldn't use regex. My point was merely to review the OPs code as is and improve his coding style. \$\endgroup\$ – dcastro Sep 2 '15 at 13:23
1
\$\begingroup\$

You only need to check what character the current run consists of at the beginning of each run.
This also allows you to adjust the index-variable one step and remove a few calculations.
Like so:

char c = source[0];
int charCount = 1;

for (int i = 1; i < source.Length; i++)
{
    if (c == source[i])

and later:

else
{
    c = source[i];
    charCount = 1;
}

Also, depending on your data, it might be faster to only check the length at the end of a run.
Like so:

if (c == source[i])
{
    charCount++;
}
else
{
    if (charCount >= sequenceLength)
    {
        return true;
    }

    c = source[i];
    charCount = 1;
}

This could be a bad idea if the last run is much longer than sequenceLength though.

\$\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.