in reply to "Biochem BINGO" or "Noob Seeks Zen Thrashing"
These are probably globals, aren't they? You want them to be available everywhere, right? I'd go with our, which is the complement of my, and still avoids having that use vars qw($people $prizes $draws); thing.my $people = 10; my $prizes = 8; my $draws = 22*2;
These don't seem to be constants, but are perhaps "today's context". There are several ways to improve this, one being to make these changeable on the command line, but have a default:
That works if you don't have other command line values to pass. Note that to change $draws, you have to provide values for the others also.our $people = (shift or 10); our $prizes = (shift or 8); our $draws = (shift or 22*2);
A little more work, if this is a quick and dirty for your own consumption only, you might just filter the command line manually:
However, see that if you really want to make it robust, this gets into a lot of code quickly. Another idea is to use one of the Getopt::* modules. I prefer Getopt::Declare, because the usage documentation generates the command line parsing code. For instance, you might come up with this:our $people = 10; our $prizes = 8; our $draws = 22*2; my @save_args; while (@ARGV) { if ($ARGV[0] eq '-people') { shift @ARGV; if (@ARGV) { my $temp = shift @ARGV; if (($temp > 0) and ($temp < 1e6)) { $people = $temp; } else { die "Error: -people out of range, <$temp>, " } } else { die "Error: -people, no value, " } next; } # etc. }
This automatically defines options such as -help that essentially prints out the option spec (without the code snippets) as the usage statement.use Getopt::Declare; our $PEOPLE_OPT = '-p'; our $PRIZES_OPT = '-z'; our $DRAWS_OPT = '-d'; our $PEOPLE_DEFAULT = 10; our $PRIZES_DEFAULT = 8; our $DRAWS_DEFAULT = 22*2; our $people = $PEOPLE_DEFAULT; our $prizes = $PRIZES_DEFAULT; our $draws = $DRAWS_DEFAULT; my $option_spec = qq{ $0 does some magic for BS Bingo, yada, yada, yada Options: $PEOPLE_OPT <people> Sets number of people (default is $PEO +PLE_DEFAULT) { reject ($people < 2) or ($people > 1e6); \$::people = $people; } $PRIZES_OPT <prizes> Sets number of prizes (default is $PRI +ZES_DEFAULT) { reject ($prizes < 2) or ($prizes > 1e6); \$::prizes = $prizes ; } $DRAWS_OPT <draws> Sets number of draws (default is $DRAWS_ +DEFAULT) { reject ($draws < 2) or ($draws > 1e6); \$::draws = $draws ; } } my $options = Getopt::Declare->new( $option_spec ) or die "\n**** Error processing command line options, terminating $0 +\n";
Jumping down to mkWinstring, you have this:
A lot of work, and not obvious to non-Perlers with the double use of $_. Why not something like this?for (0..($draws-1)){ local $_ = $seq[$_];
I actually prefer to name the loop variables, if it makes it easier to follow (especially if you're going to have nested or longer loops):for (@seq[0..$draws-1]){
Just after that, any reason to use m,B, over the more standard /B/, or even m/B/?for my $draw (@seq[0..$draws-1]){
Later you have:
which seems like a lot of punctuation compared to:my $winstring = $b[$j].' '.$i[$j].' '.$n[$j].' + '.$g[$j].' '.$o[$j]."\n";
While you'll get the infrequent "Don't interpolate variables (in double-quoted strings) when you don't need to!", I don't think there's much value when using the string concatenation operator ('.'), and it cleans up the statement nicely.my $winstring = "$b[$j] $i[$j] $n[$j] $g[$j] $o[$j]\n";
Just after that, you've inserted a piece of debug code where you then split $winstring on whitespace. Unless there's whitespace in the original values, it would have been better to do this debug work beforehand.
getRand is particularly misinformed :)
You seem to want an integer between 1 and 100 here (you'll never get 101, as int truncates towards zero). Wouldn't this work better?sub getRand(){ while (1){ my $temp = int(1111*rand()/11); unless ($temp = 0) {return $temp; exit} } }
Also,my $temp = int(rand(100)+1;
can mistakenly lead someone to believe the program exits here, after finding a suitable number. But return means the exit is never executed.{return $temp; exit}
You might consider reworking your metadata file to use numbers only, instead of the column designators. Look up the column in the code that reads this instead. It makes it easier to read in, and easier to propagate to other scripts.
Moving on to "Make Bingo Cards"...
I'm a little fuzzy here, but I think there's some magic that happens with $_ (or is it @_) when calling subs with &sub as opposed to sub(). Unless you're trying to get around one of these special cases, I'd suggest using the paren form:&doInit; &mkCards;
If these are declared before use, you can drop the parens. I would suggest keeping them, as reorganizing the script can put the use before the declaration, and Perl will gripe at you. Besides, having the parens forces the compiler to accept the lack of parameters passed, instead of guessing when they are used like this:doInit(); mkCards();
Is that getRand() + 17 or getRand(+17)?$status = getRand + 17;
You might consider using the m//x option to document your more complex regexen
Down in mkCards:if (m,([01]{10})\n((?:(?:\w\-\d+[\s\n]){5}){5} +),)
So every time through the loop, you declare a new sub exactly like the last one? Is there some reason this can't be declared elsewhere? You mention some scoping issue, but don't explain. The only variable that isn't declared in the sub is %seen, which you're using as a "seen" cache, and create anew with each loop. Why not declare the sub once, and pass in a reference to the variable, if that's required? Or better yet, create a closure with the sub and cache, and a helper sub to clear the cache when needed:local *getNewDrawBall = sub { #workaround for +scoping inner subroutine with typeglob - ugly Per +lism my $type = shift; my $temp2; while (1){ $temp2 = &getDrawBall($type); if (exists $seen{$temp2}) { next; }else{ $seen{$temp2}++;return $temp2;} #pr +int "DEBUGdraw: $temp\n"; } };
Then the top part of your loop reduces to this:{ # closure for getNewDrawBall, getNewNonDrawBall my %seen; sub clearNewDrawBallCache { %seen = (); } sub getNewDrawBall { # stuff here } sub getNewNonDrawBall { # stuff here } } # end closure for getNewDrawBall, getNewNonDrawBall
Again, you typeglob subs, for what reason?for my $k (1..$losers) { my $linestring; my $count = 0; clearNewDrawBallCache(); #SPOIL CARD HERE # etc... }
This sub doesn't reference anything not created here, except for getRand, itself a sub. Using my, there are no variable name clobbering issues (except of course with the typeglob :)local *coinFlip = sub { my $z = &getRand; my $coin = int($z)% 2; return $coin; };
Well, that's enough for now. Keep looking for improved idioms -- forms that make it easy to read, and easy to follow, and don't duplicate effort. If you code so that a non-programmer can mostly figure out what's happening (by using descriptive names, comments), and you format so that it wouldn't look like line noise if framed on your wall, you will have achieved a lot.
-QM
--
Quantum Mechanics: The dreams stuff is made of
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re^2: "Biochem BINGO" or "Noob Seeks Zen Thrashing"
by NamfFohyr (Scribe) on Dec 12, 2005 at 18:38 UTC | |
by QM (Parson) on Dec 12, 2005 at 23:10 UTC |