Too complicated code for generating a random string?

Jens Mueller jens.k.mueller at gmx.de
Sat Feb 23 06:25:42 PST 2013


monarch_dodra wrote:
> 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.

I see your point. But I believe it depends on what you want to achieve
and how you are constrained. I do not care about very good efficiency in
this case. I just need a solution that works. I can replace that code
later when needed. Your solution is less precise but much more
efficient.
Can we have a generic function/code that is as efficient as yours? That
is what we are aiming for, right?

My hope is that machine code generated from

auto randomLetter = () => letters[uniform (0, letters.length)];
auto randomString = iota(10).map!(_ => randomLetter()).array();

is on par with hand-written code most of the time. Or at least it's fast
enough for many use cases.

Jens


More information about the Digitalmars-d mailing list