BladeLogan asked:
@Sergey Alexandrovich Kryukov How would you rate this code? Do you have anything I can compare it with?
There are several problems:
It's good that you initialized only one instance of
System.Random
. But, for real randomness (this is
pseudo-random number generator), you need to
seed a random value in an algorithm. I do it this way:
Random r = new Random(System.DateTime.Now.Ticks.GetHashCode());
See also the Microsoft code sample:
DateTime Structure (System)[
^].
It's bad that you hard-coded certain things as
immediate constants. For example, 26 (and also 10). This is not supportable. What if you change your "abc..." string? You need to calculate this 26 out of the length of this string. Imagine what happens if you decide to change
character repertoire. How would you support it. This string, too, does not have to be hard-coded. You can, for example, auto-generate the array from, say, a single character, and required length of character length. More robust approach would be to break this range by first non-letter character. For that purpose, you can use
System.Char.IsLetter
:
Char.IsLetter Method (System)[
^].
So far, this is the worst of your problem, hard-coding, and, moreover, in unsupportable way. Hard-coded 10 is not that bad, but it's in the middle of the code. If you need a constants, it's always better to declarer explicit constant,
const int…
.
Let's go further.
Generation of random string works correctly. But this is a separate functionality. You need to defined it as a separate distinct unit, as type or at least a method. The instance of
Random
can be passed to this method as a parameter. It's is especially important to separate UI from other parts of code.
RandomString
is not encapsulated. Generally, you need to hide all you need for random generation in a compact unit which does not give any access to outside context. By the way, instead of "", always use
string.Empty
. No, it makes no functional difference, but using this
readonly field will help you when you get rid of
immediate constants, will make your code more supportable — see above.
Some more…
Also, never ever use names like
Form1
and
button1_Click
. It's really good to follow pretty good Windows naming conventions.
Never ever use names generated by the designer. The designers "does not know" your intentions. Those names are not designed to be left as is. And of course it's impossibly for the algorithm to generate semantically sensible name. You should take care about that. You should rename everything as you use the names. This is also about the maintainability of your code. If you look at your code few months later, you may not even understand what you did a while ago.
And finally,
randomString += …
is really bad.
You should not use repeated string concatenation; this is inefficient. The reason is: string type is
immutable. You don't even concatenate string. You create a new string instance every time. Why? Here is the solution: instead of
immutable string type, use its mutable counterpart, the class
System.Text.StringBuilder
:
StringBuilder Class (System.Text)[
^].
That's all I could spot by this time. As you can see, there are many problems, and some of them are serious enough. You need to analyze them all and learn what to pay attention for.
One more thing: your post "Solution 1" is not really a solution. Worse, why would you self-accept this "solution"? Next time, post such matter as a part of your question, using "
Improve question".
—SA