Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've written a function that searches for the nth occurrence of a character in the given string. A positive value for n will search from left-to-right, while a negative value will search from right-to-left. Zero is not a valid value for n.

Left-to-Right Search

"AABAXYAMN".NthIndexOf('A', 1) -> 0
"AABAXYAMN".NthIndexOf('A', 2) -> 1
"AABAXYAMN".NthIndexOf('A', 3) -> 3

Right-to-Left Search

"AABAXYAMN".NthIndexOf('A', -1) -> 6
"AABAXYAMN".NthIndexOf('A', -2) -> 3
"AABAXYAMN".NthIndexOf('A', -3) -> 1

Code

/// <summary>
/// Searches for the nth occurrence of a character in the given string. A positive value for n will search from
/// left-to-right while a negative value will search from right-to-left. Zero is not a valid value for n.
/// </summary>
public static int NthIndexOf(this string input, char charToFind, int n) {
    int position;

    switch (Math.Sign(n)) {
        case 1:
            position = 0;
            while (((position = input.IndexOf(charToFind, position)) != -1) && ((--n) > 0)) { position++; }
            break;
        case -1:
            position = input.Length - 1;
            while (((position = input.LastIndexOf(charToFind, position)) != -1) && ((++n) < 0)) { position--; }
            break;
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }

    return position;
}
share|improve this question

First, your loops are evil to read. It took me several minutes to figure out what the condition did. Essentially, you loop N times, where N is the Nth instance of the character in the string you wish to find the index for. Each time you loop, you find the index of the next occurrence of the character in the string, update the position to search from, and continue the process until the index is either -1 (no Nth instance), or until n > 0 or n < 0, based on which side you are searching from.

A simpler way to write this algorithm is as follows:

public static int NthIndexOfC(this string input, char charToFind, int n)
{
    int position;

    switch (Math.Sign(n))
    {
        case 1:
            position = -1;
            do
            {
                position = input.IndexOf(charToFind, position + 1);
                --n;
            } while (position != -1 && n > 0);
            break;
        case -1:
            position = input.Length;
            do
            {
                position = input.LastIndexOf(charToFind, position - 1);
                ++n;
            } while (position != -1 && n < 0);
            break;
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }

    return position;
}

It takes a little bit more room since I wrote the loop body on its own lines, but it is easy to understand, and there are no assignments in the loop condition.

However, this is still not the way I would write this algorithm. When you think about how IndexOf works, it iterates the string with a for loop from the specified starting index until it reaches the first instance of the requested character, which it returns. We can write similar behavior, but just keep track of which index we are at:

public static int NthIndexOf(this string input, char charToFind, int n)
{
    int count = 0;
    switch (Math.Sign(n))
    {
        case 1:
            for (var index = 0; index < input.Length; index++)
            {
                if (input[index] == charToFind)
                {
                    count++;
                }

                if (count == n)
                {
                    return index;
                }
            }
            break;
        case -1:
            for (var index = input.Length - 1; index >= 0; index--)
            {
                if (input[index] == charToFind)
                {
                    count++;
                }

                if (count == Math.Abs(n))
                {
                    return index;
                }
            }
            break;
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }

    return -1;
}

This is the longest solution yet, but we aren't finished. Notice all that duplicated code?

public static int NthIndexOf(this string input, char charToFind, int n)
{
    switch (Math.Sign(n))
    {
        case 1:
            return NthIndexOf(input, charToFind, n, 0, input.Length);
        case -1:
            return NthIndexOf(input, charToFind, n, input.Length - 1, -1);
        default:
            throw new ArgumentOutOfRangeException(message: "param cannot be equal to 0", paramName: nameof(n));
    }
}

public static int NthIndexOf(string input, char charToFind, int n, int searchFrom, int searchTo)
{
    int count = 0;
    var index = searchFrom;

    while (index != searchTo)
    {
        if (input[index] == charToFind)
        {
            count++;
        }

        if (count == Math.Abs(n))
        {
            return index;
        }

        index += Math.Sign(n);
    }

    return -1;
}

There. Clean, easy to understand and maintain, and lots of options to use. Notice that the second NthIndexOf needs to have the searchTo one integer larger or smaller than the value you want to stop at to accommodate the multi-directional search.

share|improve this answer
    
Not really disagreeing with your point about the loops being ugly (I can see how they'd appear that way) but if read from right-to-left I personally found them to be even simpler than the do { ... } while(...) alternative. Interesting to see that the general opinion disagrees. I'll be trying out the manual iteration using your last example as a guide; the only thing I would change at first glance is the repeated calls to Math.Sign and Math.Abs that might add up unfavorably. – Kittoes0124 12 hours ago
    
Yes, keeping them separate might be a good idea if performance is critical. – Hosch250 12 hours ago

Pretty sure iterate the string once and add occurrence is more efficient than multiple calls to input.IndexOf. It has to skip forward by position multiple times.

I tested a million runs using the examples in the question and it is 15 times as fast.

For me it is also easier to read.

public int GetPosByCount(int n, string s, char target)
{
    if (String.IsNullOrEmpty(s))
        return -1;
    if (n > s.Length)
        return -1;
    if (n > 0)
    {
        int occurance = 0;
        for (int i = 0; i < s.Length; i++)
        {
            if (s[i] == target)
            {
                occurance++;
                if (occurance == n)
                    return i;
            }
        }
        return -1;
    }
    else if (n < 0)
    {
        int occurance = 0;
        for (int i = s.Length - 1; i >= 0; i--)
        {
            if (s[i] == target)
            {
                occurance++;
                if (occurance == -n)
                    return i;  
            }
        }
        return -1;
    }
    else
    {
        return -1;
    }
}
share|improve this answer
1  
Comments are not for extended discussion; this conversation has been moved to chat. – Jamal 11 hours ago

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.