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