>>Hi all,
>>
>>I have this method I am refactoring for various reasons. There is a lot of nesting and plan to remove some of it. I am wondering about this intital test inside of a lock statement and wonder if it would be safe to move it outside of the lock construct.
>>
>>This is what it looks like now.
>>

>>public void FindActiveWebService()
>>{
>>    lock (FindWebServiceSync)
>>    {
>>        if (!IsFindingActiveWebService)
>>        {
>>            IsFindingActiveWebService = true;
>>            try
>>            {
>>                // Lots of crappy code and nested code in here with loops and such.
>>                // Planning to refactor all this junk.
>>            }
>>            finally
>>            {
>>                IsFindingActiveWebService = false;
>>            }
>>        }
>>    }
>>}
>>

>>
>>Wondering if it would be just as safe to do this and remove the extra if structure.
>>

>>public void FindActiveWebService()
>>{
>>    // Moved outside the lock
>>    if (IsFindingActiveWebService)
>>        return;
>>
>>    lock (FindWebServiceSync)
>>    {
>>        IsFindingActiveWebService = true;
>>        try
>>        {
>>            // Lots of crappy code and nested code in here with loops and such.
>>            // Planning to refactor all this junk.
>>        }
>>        finally
>>        {
>>            IsFindingActiveWebService = false;
>>        }
>>    }
>>}
>>

>>
>>Thanks for any thoughts.
>
>Presumably, in the current code, IsFindingActiveWebService is referenced outside of the FindActiveWebService() method for some purpose - if it is not then it is redundant since no other thread would get inside the lock whilst it is set true.
>
>I guess your proposed change would prevent a second thread attempting the lock maybe 99.999% of the time but there's always a chance the 'if' will execute before IsFindingActiveWebService gets set true. But when that happens it presumably would be no different to running the method consecutively with no lock contention ?

Right, but then now I know nothing else sets IsFindingActiveWebService to true or false except within the lock. Nothing is using it either though so must have been a plan in someones head that didn't get implemented. An obvioius sign of lack of design to start with along with so many other things. Funny thing, I have been told several times about how good this code is because it works. Shows how our work gets viewed, if it works it is good. No need to worry about maintainability. Sorry, just on a rant as I keep finding silly things that make me think there must have been a reason, but can't find it. Re-factoring code can be dangerous so I do it causously.

Thanks for your comments.
Timothy