|
I have no idea what he locks in this C# code.
lock (path)
{
if (File.Exists(path))
{
File.Delete(path);
}
}
|
|
|
|
|
Just in case, you know. 
|
|
|
|
|
Maybe he was afraid that someone/something (like Windows, CLR or even God) would change his path containing local string before he got to delete the file.
Does that count as paranoia? 
I have no smart signature yet...
|
|
|
|
|
Don't laugh with the guy! Maybe he is just a visionary protecting his code in the event of time travel.
|
|
|
|
|
He probably thought it was to 'lock' the file, so that someone else won't be able to get a handle to the file. 
|
|
|
|
|
|
It is possible, I suppose, that path was originally an instance variable and this was subsequently refactored to make it local to the method. That would explain it, if someone just didn't bother removing the lock on path when they did the refactoring.
It's also possible, of course, that whoever wrote it just didn't understand lock.
|
|
|
|
|
lock doesn't care whether it's an instance or local variable. It's locking the object, not the variable.
As strings with the same content might or might not be shared by the .NET runtime (depending on how the strings were created, .NET version, compilation options, etc.), no one can ever know what exactly that code was locking. But even if that originally was an instance variable, the original code would have been incorrect.
David Skelly wrote:
It's also possible, of course, that whoever wrote it just didn't understand lock.
That's the only possible explanation.
|
|
|
|
|
Daniel Grunwald wrote: That's the only possible explanation.
I'm amazed at how many people seem to think that the way to avoid problems with concurrent access to an object is to arbitrarily pick something kinda-sorta related to that object and lock it. They see all the warnings about the risks of locking publicly-available objects and try to avoid doing that, despite the fact that if a lock must be held between calls to an objects methods or properties, it must be publicly available. To be sure, it's often best if things can be arranged so a lock needn't/won't be held while running 'outside' code, but when it's necessary one shouldn't try to avoid exposing the lock.
|
|
|
|
|
supercat9 wrote: if a lock must be held between calls to an objects methods or properties, it must be publicly available.
That isn't quite true. An instance member marked private can be the lock value, manipulated solely by members of the class, like this:
class ThreadedWhatsit
{
public ThreadedWhatsit
{
lock(_Lock)
{
}
}
public ThingType PropertyA
{
set
{
lock(_Lock)
{
_PropertyA = value;
}
}
get
{
ThingType propertyA;
lock(_Lock)
{
propertyA = _PropertyA;
}
}
}
private ThingType _PropertyA = new ThingType();
private object _Lock = new object();
}
|
|
|
|
|
Gary R. Wheeler wrote: An instance member marked private can be the lock value, manipulated solely by members of the class, like this
In your example, the lock is released between calls to the object's functions. If you used the Monitor.* functions, you could have have your code hold a lock when a function returns, but doing so would be even more dangerous than publicly exposing the lock object.
The main danger of publicly exposing a lock object is that there's no way the code for the class to protect against deadlock. Having a function which returns while a lock is held poses a much bigger danger, which is that the lock might be accidentally abandoned while it is held.
|
|
|
|
|
My point is that the class manages the object the lock is protecting. All access to the object are through the class, so therefore the object is protected.
Publicly exposing a lock, or holding the lock across method calls, is insanely poor design.
|
|
|
|
|
| Publicly exposing a lock, or holding the lock across method calls, is insanely poor design.
Agreed, but if one wishes to allow people to nicely enumerate collections I'm not sure there's always a good way around it. Personally, I wish that Microsoft's contract had allowed collections to be changed during an enumeration subject to some restrictions (basically, things which exist throughout an enumeration must be returned once; things that exist for part of an enumeration may be returned at most once) but that's not how Microsoft specified it.
I am well aware that locking a collection during enumeration is a dangerous recipe for deadlock, but there isn't always a practical alternative.
|
|
|
|
|
To add to David's reply, the intention might have been to protect against two threads of the same program concurrently trying to delete the file (would work if both threads lock on the smae path instance).
Or maybe a generous hope that this gives him a somehow-transitional file system.
|
|
|
|
|
Maybe the variable "path" is only used for locking against multiple threads running this at the same time. If they assign a value to the variable, that would be kinda weird. I use the "lock" keyword for multithreading, but I usually name the variable something like "padlock" and don't use it for anything else.
|
|
|
|
|
Complements of a ball of VB6 mud:
6580 Exit Function
where 6580 is an automatically generated line number, starting from 1.
|
|
|
|
|
Wes Jones wrote: starting from 1
that is where the horror is IMO; I don't mind line numbers, when present I want them to start at 1000 and increment by 10 though, using a fixed 4-digit format, and allowing for limited inserts without having to renumber all the time.
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles]
I only read formatted code with indentation, so please use PRE tags for code snippets.
I'm not participating in frackin' Q&A, so if you want my opinion, ask away in a real forum (or on my profile page).
|
|
|
|
|
I guess somebody forgot to ask question "how many thousands of lines of code in one function is too many?"
|
|
|
|
|
Everything that's over 9000 I figure.
|
|
|
|
|
txtName being a text field:
txtName.Text.ToString() 
Noman Muhammad Aftab,
Software Mechanic
|
|
|
|
|
Yeah, that's indeed quite insecure. Obviously you should Cstr() the thing and then Ctype it to string for good practice.
|
|
|
|
|
I can't tell you how many times ive seen ToString() called on a string type. Its a typical newbie mistake, the lack of understanding of data types.
|
|
|
|
|
I wonder if the person is trying to be paranoid-protective about the possibility that a property might return a type which is not a string, but features bidirectional widening conversions? As a hypothetical example, one might have a private string-interning pool. Properties of a class which would normally return string could instead return items from the pool. If the properties returned strings, then setting one such property to another would require generating a new string object and then doing some sort of dictionary lookup to locate the string in the pool. By contrast, if the objects returned intern-pool references, those steps could be eliminated.
One hazard with such an approach is that if such an object were passed to a function that expected something of type object, things would appear to work normally (the string would be extracted when the object was cast to a string) but the object that was kept would be a pool reference. This could cause an entire pool of interned strings to be kept alive even if there were no live references to anything that cared about the pool being a pool (just a reference to something thought to be a string).
|
|
|
|
|
The sort of people who write myTextField.Text.ToString() would have no clue what you've just written.
|
|
|
|
|
J4amieC wrote: The sort of people who write myTextField.Text.ToString() would have no clue what you've just written.
Naturally. On the other hand, they might have heard of some vaguely-similar situation in which a competent programmer who did understand what was going on had applied some seemingly-unnecessary cast, and figured that if a cast was needed there and they didn't understand any distinction between that case and the one at hand, they should apply a cast (or explicit ToString) in the present case as well.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
