Reentrant code and local variables

Question

I have a method like:

private static string AmpRemove(string str)
{
    int index = str.IndexOf('&');
    if (index > 0)
        str = str.Substring(0, index);
    return str;
}

Here I am trying to get text from a string until & character is found. My senior modified this method to

private static string AmpRemove(string str)
{
    if (str.IndexOf('&') > 0)
        str = str.Substring(0, str.IndexOf('&'));
    return str;
}

So instead of storing the index, it will calculate it twice, and his reasoning was that since that method would be called in multiple threads, there could be invalid value stored in index.

My understanding of thread is limited, but I believe each thread would have its own stack, where the parameter str and index would be pushed. I have tried reasoning with him, that this is a reentrant code and there is no way that multiple threads could modify a local variable in this method.

So, my question is, Am I right in assuming that caching storing index is a better solution, since it will not involve calculating index twice and since it is local variable and str is a parameter local to method, there is no way that multiple threads could modify/change str and index ?

Is that correct ?

Answer 1

index cannot be modified given the code, because it is a local variable, and any method called from another thread is a separate call and thus does not share local variables.

However, you do not control the string parameter since it gets passed in. This would make it thread unsafe... Except according to MSDN, a string object is immutable. You can thus assume that the string you get passed remains the same between calls, and is thread safe. Although you reassign the value of str, the parameter was not passed as a ref and therefore that only reassigns the local pointer, it does not modify the caller's variable which was passed in.

Thusly, the first code, as presented, is thread-safe. Any changes to the type being passed in or how it's passed in cannot be immediately assumed thread-safe.

Answer 2

Your original method is threadsafe. But:

1. Reassigning method variables can be confusing, you could just return the Substring: private static string AmpRemove(string str) { var index = str.IndexOf('&'); if (index > 0) { return str.Substring(0, index); } return str; }

2. If the '&' is at index 0 your code will return the whole string. Is that what should happen, or should it be index >= 0?