In your synopsis you have this line:
my $random_thing = random($hash_of_arrays, $list, $options);
but in reading the source it appears that the second argument is expected to be a scalar, not a list. That's confusing.
Also you say several times, "When XXX is selected" but give no clue as to what "selected" means in this context. Again, it is only by reading the source that we can see you mean "passed as a literal value as the second argument to random()".
I think it would help a great deal if in your snippet listing the example random_color() sub you were actually to specify values for the three variables instead of leaving us all to wonder what they might be (or even what form they should take). eg.
my $key = 'foo';
my $additions = 'elephant';
my $color_hash = { a => [qw/ I do not know /], b => [qw/ what should
+ go here /] };
my $rv = random_color ($key, $additions);
sub random_color {
my ($color_key, $color_additions) = @_;
random($color_hash, $color_key, { caller => 'random_color', additi
+ons => $color_additions});
}
Finally, I don't see why instant_rand() is so named. Why not list_item_rand()?
HTH.
| [reply]
[d/l]
[select] |
- $color_hash magically appears in your random_color() example usage, and you give no indication of where it comes from (it's not a parameter to your example sub) or what it should look like (other than an arbitrary "hash(ref) of arrays")
- It takes multiple readings of the POD for me to guess that the data, keys, by keys, ... under the random() function refer to string values of the $color_key argument. Make sure you put quotes around each of the strings, but not around the undef.
- For tiny_rand(), you might want to mention "a coin flip", and give an example, like tiny_rand("HEADS", "TAILS");. The name is not helpful; maybe rename (or alias) to random_from_two()
- For instant_rand(), give an example, like instant_rand("HEADS", "TAILS", "EDGE", "EMBED IN CEILING");. Maybe rename (or alias) to random_from_list().
| [reply]
[d/l]
[select] |
Hi Lady_Aleena,
I started by downloading your .pl code and then generating the HTML. I only looked at the HTML doc, not your code in this review. Looking at the code would perhaps solve lots of questions, but that is not the point of a doc review.
I don't understand the main purpose of this module other than it has something to do with randomizing things. Randomize what? And why? How does your module do much more than obvious uses of the standard Perl rand functions?
Why is there a separate tiny_rand() from instant_rand()?
Why the need for 2 functions? As for naming why wouldn't a "tiny" one be "instant"?
As a naive person to your topic, I just do not understand what this thing is supposed to do.
I am not trying to "rain on your parade". But, I think the doc's could be improved. superfluous comment deleted.
update: Starting to look at the actual code and I agree with Re: RFC: Proofread POD for my main random module, please? (more) from pyrt in regards to "tiny_rand()" and "instant_rand()". I would think random_from_list() as pyrt suggests is more understandable. I don't see a reason to expose the 2 parameter version of "instant_rand()" as "tiny_rand()" unless there is some significant performance reason to do so. | [reply] |
Changes
- Broadened examples.
- Example usage with each section of POD.
- tiny_rand and instant_rand now aliases for random_from_list (name suggested by pryrt and seconded by Marshall, though hippo suggested another name).
- Current name restored, though I am still interested in help on renaming it.
About the names tiny_rand and instant_rand and why tiny_rand wasn't just expanded when I came up with instant_rand, they seemed like good idea at the time. On reflection, they are not. So your suggestions were taken and used. However, random_from_list doesn't flow out of the fingertips as quickly as tiny_rand does.
pryrt, when you said don't quote undef, please know I didn't "quote" anything. I used the C<> POD markup around the words that are quoted in perldoc view. So C<undef> becomes "undef" in the perldoc viewer. In CPAN's POD checker there are no quotes unless I typed them into the POD.
I hope the expanded examples help, though the POD is now so very very long for such short code.
No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!
Lady Aleena
| [reply]
[d/l]
[select] |
Personally, I think that this is a great improvement over the original and that the examples add much to the clarity of purpose of the module as a whole. Well done.
Now that it's clear (to me) what the module's purpose is, I would suggest changing the nameline from "RolePlaying::Random is a set of tools to generate randomness." to something like "Randomly select values from sets of lists.". I think that this more accurately reflects what the module does. It certainly doesn't generate randomness, that's for a PRNG to perform. What it does is take randomness generated by other code and apply it in various ways to supplied datasets.
random_from_list doesn't flow out of the fingertips as quickly as tiny_rand does.
Maybe not. This helps to illustrate one of the differences between coding for yourself and coding for others. In the latter case, clarity wins every time. Naming your subs (and other symbols such as packages, variables and constants) to follow their purpose helps make the code which others write using them to be legible and maintainable. If you (or anyone) really wants then in their own code to use a shorthand for a sub they can by aliasing or subclassing or whatever. But careful choice of symbol names improves the chances of code being self-documenting and that's a good target to aim at.
FWIW, I seen nothing in this code which ties it to the RolePlaying space. It seems perfectly general. I've just looked around the namespaces and there's neither List::Random nor Random::List. There is, however, Random::Set which seems to perform a similar task but without your bells and whistles. If you think that Random::Set is no more general than your module then maybe consider a more general name for yours. Don't forget PrePAN for soliciting ideas too.
| [reply] |
++ Much improved. My point with mentioning "quotes" was I was hoping When C<all> or C<undef> is selected would be When C<'all'> or C<undef> is selected (and similarly for other textual literals, like C<'by keys'> or C<'keys'>). But with the examples that now follow, that's quickly clarified, so it's not critical.
nitpick #1: random_from_list should be included in @EXPORT_OK
nitpick #2: in the =head4 keys section, there are typos of "randon" and "randon_color"
I agree with hippo that List::Random and Random::List both seem to apply as reasonable names. Given Random::Set and Random::Day, which both select a random element from a group, I think I'd have a slight preference for Random::List, because your module complements those nicely. (I agree with your OP, in that taking the top spot of package Random; is not appropriate.) I also ++agree with hippo's Synopsis suggestion.
Don't worry about long POD: in my opinion, long-but-helpful is much better than short-and-unhelpful. As a new user to a module, I want it descriptive enough that someone who doesn't have the author's history with the module would be able to read the documentation and immediately grasp what the module is about and how to use it.
| [reply]
[d/l]
[select] |
Changes...
- Relocated module from RolePlaying name space to Fancy name space.
- Renamed
- module from Random to Rand
- random to fance_rand
- random_from_list to fancy_rand_from_array (a hash is also a list)
- Corrected errors seen by others.
- Added uniq to the flattened hash list so repeated items don't get more weight.
You might wonder why Fancy::Rand. Well, I already have Fancy::Map, Fancy::Splice, Fancy::Split, Fancy::Join::Defined, and Fancy::Join::Grammatical. I decided when I do something "fancy" with a perl function, I will put it in the name space Fancy and name the top subroutine fancy_function. So, now I can add Fancy::Rand and fancy_rand to my list of Fancy::Functions.
Here is the final rough draft. I think I got everything fixed.
package Fancy::Rand;
use strict;
use warnings FATAL => qw(all);
use Exporter qw(import);
our @EXPORT_OK = qw(fancy_rand fancy_rand_from_array tiny_rand instant
+_rand);
use List::Util qw(shuffle uniq);
sub fancy_rand {
my ($list, $user_input, $opt) = @_;
my $random_item;
if ($user_input && $user_input =~ /(?:help|options)/) {
my $keys = join("\n ", sort keys %{$list});
$random_item = "Your options are:
$keys
by keys: get a random key
all: get a random item from any key on the list
keys: get the list of the keys
data: get the whole hash";
}
elsif ($user_input && $user_input eq 'data') {
$random_item = $list;
}
elsif ($user_input && $user_input eq 'keys') {
$random_item = [keys %$list];
}
else {
my @random_list;
if ($user_input && $user_input eq 'by keys') {
@random_list = keys %{$list};
}
elsif (!$user_input || $user_input eq 'all' ) {
@random_list = uniq(map { @$_ } values %{$list});
}
elsif ($list->{$user_input}) {
@random_list = @{$list->{$user_input}};
}
else {
my $caller = $opt->{caller} ? " in ".$opt->{caller} : 'fancy_ran
+d';
die "Your option '$user_input' is not a list $caller.\n\tStopped
+$!"
}
push @random_list, @{$opt->{'additions'}} if $opt->{'additions'};
@random_list = shuffle(@random_list);
$random_item = $random_list[rand @random_list];
}
return $random_item;
}
sub fancy_rand_from_array {
my @rand_array = shuffle(@_);
return $rand_array[rand @rand_array];
}
sub tiny_rand { fancy_rand_from_array(@_); }
sub instant_rand { fancy_rand_from_array(@_); }
=head1 NAME
B<Fancy::Rand> selects random items from sets of lists.
=head1 SYNOPSIS
use Fancy::Rand qw(fancy_rand fancy_rand_from_array tiny_rand instan
+t_rand);
my $random_thing = fancy_rand($hash_of_arrays, $selected_list, { add
+itions => [@additional_items], caller => $caller });
my $fancy_rand_from_array = fancy_rand_from_array(qw(red yellow gree
+n cyan blue magenta black white));
=head1 DESCRIPTION
B<Fancy::Rand> has two tools to help you select where your randomness
+comes from: C<fancy_rand> and C<fancy_rand_from_array>.
=head2 fancy_rand
C<fancy_rand> takes a hash of arrays and returns a random result based
+ on the list selected from the hash. It is meant to be used in the cr
+eation of other random subroutines.
my %colors = (
'general' => [qw(red yellow green cyan blue magenta white black gr
+ay)]
'eye' => [qw(amber black blue brown gray green hazel red violet)]
+,
'hair' => [qw(auburn brown black blond gray red white)],
'rainbow' => [qw(red orange yellow green blue indigo violet)],
);
sub random_color {
my ($selected_color_key, $color_additions) = @_;
fancy_rand(\%colors, $selected_color_key, { additions => $color_a
+dditions, caller => 'random_color' });
}
=head3 Selections
=head4 all or nothing
When C<all> or C<undef> is selected or nothing is entered, C<random> w
+ill flatten the hash and return any value from the hash.
Using C<fancy_rand> by itself:
my $random_color_one = fancy_rand(\%colors);
my $random_color_two = fancy_rand(\%colors, undef);
my $random_color_all = fancy_rand(\%colors, 'all');
Using the newly created C<random_color>:
my $random_color_one = random_color();
my $random_color_two = random_color(undef);
my $random_color_all = random_color('all');
All of the above will return any color in the hair, eye, and rainbow l
+ists.
=head4 a key
When a specific key is selected, C<fancy_rand> will return an item fro
+m the selected key.
Using C<fancy_rand> by itself:
my $random_hair_color = fancy_rand(\%colors, 'hair');
my $random_eyes_color = fancy_rand(\%colors, 'eye');
Using the newly created C<random_color>:
my $random_hair_color = random_color('hair');
my $random_eyes_color = random_color('eye');
=head4 by keys
When C<by keys> is selected, C<fancy_rand> will return a random key.
Using C<fancy_rand> by itself:
my $random_color_key = fancy_rand(\%colors, 'by keys');
Using the newly created C<random_color>:
my $random_color_key = random_color('by keys');
Both of the above will return a random key from C<%colors>: hair, eye,
+ rainbow.
=head4 keys
When C<keys> is selected, C<fancy_rand> will return the list of keys f
+rom the hash to help remind you of your key options.
Using C<fancy_rand> by itself:
my $random_color_keys = fancy_rand(\%colors, 'keys');
Using the newly created C<random_color>:
my $random_color_keys = random_color('keys');
Both of the above will return a list of the keys: C<['hair', 'eye', 'r
+ainbow']>.
=head4 data
When C<data> is selected, C<fancy_rand> will return the entire hash so
+ you can refresh your memory of what is in the hash.
Using C<fancy_rand> by itself:
my $random_color_data = fancy_rand(\%colors, 'data');
Using the newly created C<random_color>:
my $random_color_data = random_color('data');
=head4 help or options
When C<help> or C<options> is selected, all of your options will be li
+sted.
Using C<fancy_rand> by itself:
my $random_color_help = fancy_rand(\%colors, 'help');
my $random_color_opts = fancy_rand(\%colors, 'options');
Using the newly created C<random_color>:
my $random_color_help = random_color('help');
my $random_color_opts = random_color('options');
=head3 Optional parameters
There are two optional parameters you can use, C<additions> and C<call
+er>.
=head4 additions
C<additions> is a list that you want to add to the list you have alrea
+dy selected.
You might want to add the colors pink, blue, and purple to the choices
+ of hair color or yellow and bloodshot to eye color.
Using C<fancy_rand> by itself:
my $random_hair_color = fancy_rand(\%colors, 'hair', { additions =>
+[qw(pink purple blue)] });
my $random_eyes_color = fancy_rand(\%colors, 'eye', { additions =>
+[qw(yellow bloodshot)] });
Using the newly created C<random_color>:
my $random_hair_color = random_color('hair', [qw(pink purple blue)])
+;
my $random_eyes_color = random_color('eye', [qw(yellow bloodshot)])
+;
=head4 caller
C<caller> can be used to create an error message with the specific sub
+routine your are using random in.
Using C<fancy_rand> by itself:
my $random_color = fancy_rand(\%colors, 'rainboe', { caller => 'rand
+om_color' });
Using the newly created C<random_color>:
my $random_color = random_color('rainboe');
If the user selects a list that will not produce a result, the error m
+essage from both of the above will read as follows.
Your option 'rainboe' is not a list in random_color.
Stopped at ...
=head2 fancy_rand_from_array
C<fancy_rand_from_array> takes an array and returns a random result. C
+<tiny_rand> and C<instant_rand> are aliases for C<fancy_rand_from_arr
+ay>.
my $fancy_rand_from_array = fancy_rand_from_array(qw(black white red
+ yellow green cyan blue magenta));
my $tiny_rand = tiny_rand(qw(black white));
my $instant_rand = instant_rand(qw(red yellow green cyan blue magent
+a));
=head1 AUTHOR
Lady Aleena
=cut
1;
No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!
Lady Aleena
| [reply]
[d/l]
[select] |