|
The best line in this function is Beep()!
|
|
|
|
|
We aren't in the Lounge now - no need to censor your language...
Did you know:
That by counting the rings on a tree trunk, you can tell how many other trees it has slept with.
|
|
|
|
|
The code isn't testing for alphanumeric--just alphabetic. That having been said, I don't see anything overly hideous. I would expect VB to recognize the string with the letters and backspace as a constant, so the code simply searches for an input character in a constant string. Not gorgeous (using a named constant called AllowableKeys or something would help), but the list of allowable characters can be easily adjusted by changing the string. If upper and lower case are going to be handled identically elsewhere, it might have been nice to convert to uppercase and just check against uppercase letters.
As for my own style, if I were just checking against one contiguous range of characters, I'd use the >= and <= to check the range. With two ranges, or one range plus one or two other characters, I'd still probably use those. With more than that, I'd likely just search in a string with all valid characters.
|
|
|
|
|
supercat9 wrote: The code isn't testing for alphanumeric--just alphabetic
Yes, Alfabetic I meant, and don't forget the backspace
|
|
|
|
|
This looks like it was converted from an old dialect of Basic. The only thing I see as "odd" is the & Chr(8) , since the check for < 32 would catch all the common Ctl chars, including Bksp , anyway. But my guess is that it was converted and modified a couple of time to end up with that.
The Dim Index As Short = textBox1.GetIndex(eventSender) line looks like something left over from a previous mod or from some debugging effort. It doesn't look like Index is used at all.
CQ de W5ALT
Walt Fair, Jr., P. E.
Comport Computing
Specializing in Technical Engineering Software
|
|
|
|
|
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
|
|
|
|