Re: random string problem
You shouldn't be using the code as it is.
The documentation has this to say:
Quote:
The random number generation starts from a seed value. If the same seed is used repeatedly, the same series of numbers is generated. One way to produce different sequences is to make the seed value time-dependent, thereby producing a different series with each new instance of Random. By default, the parameterless constructor of the Random class uses the system clock to generate its seed value, while its parameterized constructor can take an Int32 value based on the number of ticks in the current time. However, because the clock has finite resolution, using the parameterless constructor to create different Random objects in close succession creates random number generators that produce identical sequences of random numbers.
In other words, creating r, r1, and r2 at the same time practically guarantees they won't produce unique random numbers.
It's generally not worth it to create more than one Random object anyway. To that end, it shouldn't ever be a variable local to a method. I'd have written it like this:
Code:
Public Class Form1
Private Shared ReadOnly _rng As New Random()
Private Function GenerateString(...)
...
I'd then use _rng wherever your code was using r and the other variables.
Here's some other comments since you want it as effcient as possible:
- ToUpper() has to perform work. The code would use less CPU if you hard-coded the upper-case alphabet.
- It'd be more efficient if you exploit that upper/lowercase characters fall in specific ASCII ranges. I won't discuss this since you asked to keep things simple; it's a more advanced topic.
- Concatenating strings with "&" is not very efficient. Strings are arrays in memory, and you can't add a new item to an array. The "&" operator creates a new string with 1 extra character, copies the old characters inside it, then adds the new character. The StringBuilder class is a better choice when you're building a string with lots of concatenation.
Optimizing this further would not be easy to explain simply.
Re: random string problem
This is a much more compact password generator, by the way:
Pass Integer for desired length.
65 is an ASCII code for A and 122 is an ASCII code for z, the maximum is desired maximum + 1
vb Code:
Public Function GenPassword(ByVal Length As Integer) As String
Dim rng As New Random
Dim pass As String = ""
For i As Integer = 0 To Length - 1
pass &= Chr(rng.Next(65, 123))
Next
Return pass
End Function
Re: random string problem
3 problems.
First, you're not using StringBuilder so I don't believe any claims of being more efficient. Array copies are pretty expensive.
Second, you're creating new Random instances with a time-based seed. Odds are you'll get the same password a few times in a row if you try to generate passwords in a loop; these odds get greater as computers get faster but I noticed this problem way back on a 400Mhz Pentium II.
Third, you're ignoring that the range 65-122 also includes [, \, ], ^, _, and `. These characters weren't mentioned originally, and many systems are silly and won't let you use them as passwords. This is what I was alluding to when I said you could use the ASCII ranges but doing it right wasn't simple.
Re: random string problem
This was mere an example. I was aware of your points about seeding - noticed it on the test run. By providing this function I assumed that it is not meant to be called frequently (typical situation is when a new user is created). If you have to generate passwords for several hundred thousand users the difference in resource consuming will be noticeable, but I'd never imagined such a scenario.
And yes, the fact that there are some other characters in the ASCII range I provided completely splitted out of my mind.
Nonetheless, it can be easily re-written taking into account all three points you've mentioned.
Re: random string problem
Thanks Sitten Spynne and cicatrix,
I will implement(sp) this in my code :
Code:
Private Shared ReadOnly _rng As New Random()
What does shared readonly mean ? Is this what
Quote:
make the seed value time-dependent
So that it creates everytime a new seed (true random?)?
I thought I needed a timer for to make it time-dependent. But then I again,
I'm a beginner. And I understand that one random variable should be enough.
Also change ToUpper to "ABCDEF...", that's easy, and more efficient, thanks !
I will wait with ASCII chars, I know what they are, but it's a little too much now.
I will ask about them later, and that code cicatrix posted looks very nice (and short),
so (when I understand ASCII) I'll only have to filter those chars ([, \, ], ^, _, and `) out,
so it generates only upper and lower chars and the numbers ?
And StringBuilder is what I need to learn also, it uses less memory then & or +
(concatenate ? need to look up/translate what that word means !)
as I understand, not that this code will use lots of memory,
but I want to learn to write code as best as possible for in the future for all my code-writing (coding).
Thanks again u2, it helped me alot !
But...
the question I had about the code using r2 and nowhere is it assigned,
that's what I thought was really strange (an error in coding as you will).
Say you dim(declare) apples and oranges (example)
but you never use the oranges anywhere in the code, why would you delcare it then ?
I think it's unnescesary(sp) use of space (in code and memory).
And what is also a good tip, is to use names for declerations as short as possible (as long as I know what they mean) like :
Dim GenerateString as ...
could be shorter like this :
Dim GenStr as...
that saves space too.
OK, bye and thanks :thumb:
Re: random string problem
Shared means the property is shared between other class instances and is only created once.
Readonly means the property is readonly and can only be assigned a value when the object is constructed.
Calling new Random() (as opposed to new Random(seed)) is how the system clock is used as seed.
I don't use VB.Net but here is what I imagine it could look like...
Code:
Shared Public Function GenPassword(ByVal Length As Integer) As String
Dim Alphabet as String = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890"
Dim Chars() as Char(length-1)'<~~not sure about the -1
For i As Integer = 0 To Length - 1
Chars(i)= Alphabet(_rng.Next(Alphabet.Length))'<~~ I guess this works in VB??
Next
Return new String(Chars)
End Function
Tweaked to avoid string concatenating and I've hardcoded the alphabet. I'm taking a guess that a char array might be more efficient than Stringbuilder when single chars are concerned.
1 Attachment(s)
Re: random string problem
I'll field the new questions. Fair warning, I'm going to be harsh on the last point because it's something you need to learn through tough love.
Quote:
Originally Posted by
San76
What does shared readonly mean ? Is this what...
Shared maps to "static" in many other languages. The idea is it says "every instance of the class that contains this variable uses the same variable". For example, let's say you put the variable in Form1 and your application creates two different Form1 instances. If you don't use Shared, there will be two different Random objects; one in each class. If you use Shared, there will only be one Random object even if you create 1,000 Form1 instances. Creating multiple Random instances is a bad practice.
Note that in a Module, all variables are automatically Shared and you aren't allowed to use the word Shared.
ReadOnly is valid whether or not Shared is present. If a variable is ReadOnly, you can only assign a value inline (I did it this way) or in a constructor. For Shared ReadOnly variables, you have to use the type constructor (Shared Sub New). After construction has passed, it is illegal to put that variable on the left-hand side of an assignment. This ensures I won't ever accidentally assign a new Random object to the variable.
Quote:
So that it creates everytime a new seed (true random?)?
I thought I needed a timer for to make it time-dependent...
No. A computer's random number generator is more or less a state machine. It requires a seed value to tell it which state to start in. If you peek at the Random constructors, you'll see you have two choices. One lets you specify the seed you want to use; this can be useful in debugging/testing because if you use the same seed every time you get the same numbers every time. The one that doesn't take any parameters says it uses a "time-dependent default seed value". It looks at the system time and uses that to generate a seed. In released programs, you'll use the time-dependent value.
Quote:
I will wait with ASCII chars, I know what they are, but it's a little too much now.
I will ask about them later, and that code cicatrix posted looks very nice (and short),
so (when I understand ASCII) I'll only have to filter those chars ([, \, ], ^, _, and `) out,
so it generates only upper and lower chars and the numbers ?
I can actually prove that using ASCII is harder to understand, harder to maintain, and possibly not as performant. I'll come back to this one.
Quote:
the question I had about the code using r2 and nowhere is it assigned,
that's what I thought was really strange (an error in coding as you will).
...
I think it's unnescesary(sp) use of space (in code and memory).
Correct. You shouldn't declare variables you don't plan on using. It wastes memory and clutters your code.
Quote:
And what is also a good tip, is to use names for declerations as short as possible (as long as I know what they mean) like :
Dim GenerateString as ...
could be shorter like this :
Dim GenStr as...
that saves space too.
.
NO. NO. NO. NO. A thousand times NO. This made the hair on the back of my neck stand up and somewhere I know a kitten died because you suggested it.
"GenerateString" is 14 characters. "GenStr" is 6. That's a savings of 8 characters, 16 bytes since Windows uses UTF-16 in memory. I've got 4GB of RAM on my machine; so I've saved myself 0.000000037% of the memory on my system. But what has it cost me?
If GenStr() is in a very active part of the code and I have to read it a lot, it's going to cost me a lot. In particular, it's going to cost a lot if there are other people on my team and they have to ask me what "GenStr" stands for. What if the program has a "GeneralStore" class? Now they aren't which one "GenStr" means. This is going to manifest itself as time lost as people are confused. Worse, if there are two similar classes with similar names and similar members, it's possible someone will use the wrong one and lose a lot of work. I might cost my team $100,000 worth of effort to save an insignificant amount of memory. This is worthless, even if it could reduce the memory footprint of your application by a large margin. It won't.
The release version of the application doesn't even really keep most of the names you use. Go poke around places that let you download source and you'll find that the source code to applications is usually at least 10% larger than the application itself. In that case, most of your "optimization" won't make a difference at all.
But don't take my word for it. Write a program with nice variable names. Build it. Note its size. Now use Find/Replace to make them all smaller. Build it and check the size again. Was there a significant change? I'd wager not.
You write code once. You have to read it many times. When you make changes that hurt readability, you make reading the code harder. When it comes to real-world applications, you usually spend 20% of the time before the product releases and 80% of the time working on it after release. It makes no sense to do anything that makes that 80% of your effort harder.
There is ONE exception to this rule: if you have measured the performance of the readable code and it fails to meet some performance metric you have, then it is OK to break readability IF AND ONLY IF you also measure the new implementation to show that it meets the requirement.
Now, about the implementation I had promised. I was curious about the performance of various implementations, so I write an application to test three cases. The application compares three algorithms for generating passwords that use a mix of uppercase and lowercase letters. You can control the number of characters in each password, the number of passwords that will be generated, and the number of passwords that will be displayed via the constants at the top. I chose to generate 100,000 18-character passwords and display 5 of them because that's what it took to get some interesting runtime numbers out of my machine. The "Readable" case has a loaded name but uses a simple array that contains all of the letters I want to use. The "Concatenating" case does the same thing but doesn't use a StringBuilder. The "Memory Saver" case forgoes the array and uses ASCII values to generate the passwords. The "LINQ" case is the class clown thrown in to show that "lines of code" isn't a good measure for how efficient your algorithm is.
I've run a lot of test cases, and in order of speed they are "Memory Saver", "Readable", "Concatenating", "LINQ" but that's not the whole story.
"LINQ" isn't worth discussing at all; it's nearly 25x slower than any of the others. There are cases where removing lines of code can make your program faster, more efficient, and easier to read. This is not one of them.
"Concatenating" is a clear loser; it's usually takes close to 2x as long as the other two to finish. As I said, array copies are expensive. If we went as far as doing a memory profile, you'd see that every iteration creates NumberOfCharacters - 1 intermediate strings. With my numbers, that's a waste of 171 characters per iteration or 342 bytes, for a total waste of 32.6 MB over the life of the program. Horrid.
"Memory Saver" and "Readable" is a more interesting comparison. On my machine, these two cases generally take an average of 150-200ms to complete. On average, "Memory Saver" is 10ms faster. Is this significant though? It takes 10ms longer to generate 100,000 passwords; that's a difference of about 1ns per string. At that rate, it'd take a billion iterations for "Memory Saver" to be 1s faster than "Readable". I wouldn't call that impressive. It gets worse when you consider maintainability. What if you decide you want to allow digits? For "Readable", you'd just add 0-9 on the end of the alphabet string. For "Memory Saver", you'd have to look up the ASCII value of 0, make the RNG go from 0-61, and map characters 52-61 to the digits based on those values. Already, the optimization is making it tougher to understand. It gets worse if you want to allow a few punctuation characters, let's say "!,".[]_{}". That's some characters from three different ranges of ASCII; the corresponding numeric values are "33 44 46 91 93 95 123 125". Again, I'd just add them to the end of the "Readable" case. For the ASCII-based case, it'd take much more complicated logic to do the mapping; it'd amount to adding 9 to the RNG's range and manually mapping each of the values. Suddenly the simple password generator would balloon to 30+ lines of code. That code's going to get compiled to MSIL. At what point is this effort worth it to save the 104 bytes hard-coding the alphabet would use? I'd argue "never."
The takeaway here is there isn't always a good answer to "What's the best algorithm?" As I discussed before, you have to decide if you're looking to save time, memory, CPU, or readability and make adjustments accordingly. The ASCII-based suggestion is just fine; it works as well as my string-based suggestion and if either one is faster it's imperceptibly so. But problems creep in as the requirements change, and one is more suited to change than the other.
There's no silver bullet. "Less lines of code" can mean "more wasteful". Techniques that increase performance in one case may destroy it in others. Optimizing memory might make you use more CPU. Optimizing CPU might cost more memory. Parallel code can dramatically decrease real time but cost more memory and CPU. All of these optimizations might hurt readability, which can increase the cost of maintenance and increase the likelihood of error. They call it Computer *Science*, but in many ways it is an art.
Re: random string problem
Two cents
Code:
Public Class Form1
Private Sub Button1_Click(ByVal sender As System.Object, _
ByVal e As System.EventArgs) Handles Button1.Click
Dim stpw As New Stopwatch
Dim passGen As New RandPassGen
stpw.Reset()
stpw.Start()
For x As Integer = 1 To 10000000 'generate 10,000,000 passwords
Dim s As String = passGen.NewPass
'Debug.WriteLine(s)
Next
stpw.Stop()
Label1.Text = stpw.ElapsedMilliseconds.ToString("n0")
End Sub
Class RandPassGen
Private Shared ReadOnly _PRNG As New Random
'define the characters to be used in the password
Private Shared ReadOnly _UseChars As New System.Text.StringBuilder("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz1234567890")
Private Shared ReadOnly _MinChars As Integer = 10 'minimum password size
Private Shared ReadOnly _MaxChars As Integer = 10 'maximum password size
Private _pass As New System.Text.StringBuilder
Public Function NewPass() As String
Dim sz As Integer = _PRNG.Next(_MinChars, _MaxChars)
Me._pass.Length = 0 'new password
For idx As Integer = 1 To sz 'generate the proper amount of characters
Me._pass.Append(_UseChars(_PRNG.Next(0, _UseChars.Length))) 'randomly selected from characters to use
Next
Return Me._pass.ToString
End Function
End Class
End Class
Re: random string problem
ThtWrks2. I c u also svd sm mmry by usng shrt nms 2. Gud chce. Thse 7 bytes mght hve mde it not fit.
Re: random string problem
@sitten - srry if ur ofndd by my chc of nms. if u wld lk to hv a dscsn bout tht tpic feel free 2 strt 1.
