Too complicated code for generating a random string?

[monarch_dodra]

On Saturday, 23 February 2013 at 13:02:38 UTC, Jens Mueller wrote:
>
> I think this version also looks good:
> void main()
> {
> 	auto randomLetter = () => randomSample(letters, 1, 
> letters.length).front;
> 	writeln(iota(10).map!(_ => randomLetter()));
> }
>
> Which makes me think Phobos is convenient enough in this use 
> case. Just
> finding a simple way to use it can be a burden. It should be 
> improved
> with better documentation.
>
> Jens

No offense, but I think that's horrible. No matter how you put 
it, you should keep in mind that the strength of these ranges is 
being able do stuff lazily.

The final product of what you have is a horrible contraption of 
chained ranges, that go out of their way just to pick a random 
char for you.

Not to mention, you are creating a randomSample per character, 
which is simply not the way it is designed to be used. Generating 
an entire new range just to generate a random letter...? At 
least: use uniform instead:

auto randomLetter = () => letters[uniform (0, letters.length);

Still, even with that, IMO, the end product is bug prone, not 
very clear, and horribly inefficient. I'd hate to be the one to 
have to maintain this down the pipe... And at the end of the day, 
you'd still need to add an "array" at the end if you want to put 
it in a string anyways.

--------
Honestly, what happened to doing it by hand? It's more robust, 
cleaner, easy to understand...


     string randomString;
     {
         auto app = Appender!string();
         foreach(_ ; 0 .. 10)
             app.put(letters[uniform(0, letters.length)]);
         randomString = app.data;
     }

This is the "easy and safe" version. You could make it even more 
simple with a pre-allocate dchar:


     string randomString;
     {
         auto tmp = new dchar[](10);
         foreach(ref c ; tmp)
             c = letters[uniform(0, letters.length)];
         randomString = tmp.to!string();
     }

Or, if you know your letters are all ascii, it gets even more 
simple and efficient:

     string randomString;
     {
         auto tmp = new char[](10);
         foreach(ref char c ; tmp)
             c = letters[uniform(0, letters.length)];
         randomString = cast(string)letters;
     }

All those scope blocks I put in are not mandatory. At the end of 
the day, it is 4 lines of code. Very efficient, and the intent is 
crystal clear.

I know it's not very exciting code, they do say good code is 
boring code.