in reply to "Biochem BINGO" or "Noob Seeks Zen Thrashing"

I've got a little time, so I'll give you a few specific suggestions:
my $people = 10; my $prizes = 8; my $draws = 22*2;
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.

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:

our $people = (shift or 10); our $prizes = (shift or 8); our $draws = (shift or 22*2);
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.

A little more work, if this is a quick and dirty for your own consumption only, you might just filter the command line manually:

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. }
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:
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";
This automatically defines options such as -help that essentially prints out the option spec (without the code snippets) as the usage statement.

Jumping down to mkWinstring, you have this:

for (0..($draws-1)){ local $_ = $seq[$_];
A lot of work, and not obvious to non-Perlers with the double use of $_. Why not something like this?
for (@seq[0..$draws-1]){
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 my $draw (@seq[0..$draws-1]){
Just after that, any reason to use m,B, over the more standard /B/, or even m/B/?

Later you have:

my $winstring = $b[$j].' '.$i[$j].' '.$n[$j].' + '.$g[$j].' '.$o[$j]."\n";
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.

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 :)

sub getRand(){ while (1){ my $temp = int(1111*rand()/11); unless ($temp = 0) {return $temp; exit} } }
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?
my $temp = int(rand(100)+1;
Also,
{return $temp; exit}
can mistakenly lead someone to believe the program exits here, after finding a suitable number. But return means the exit is never executed.

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"...

&doInit; &mkCards;
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:
$status = getRand + 17;
Is that getRand() + 17 or getRand(+17)?

You might consider using the m//x option to document your more complex regexen

if (m,([01]{10})\n((?:(?:\w\-\d+[\s\n]){5}){5} +),)
Down in mkCards:
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"; } };
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:
{ # closure for getNewDrawBall, getNewNonDrawBall my %seen; sub clearNewDrawBallCache { %seen = (); } sub getNewDrawBall { # stuff here } sub getNewNonDrawBall { # stuff here } } # end closure for getNewDrawBall, getNewNonDrawBall
Then the top part of your loop reduces to this:
for my $k (1..$losers) { my $linestring; my $count = 0; clearNewDrawBallCache(); #SPOIL CARD HERE # etc... }
Again, you typeglob subs, for what reason?
local *coinFlip = sub { my $z = &getRand; my $coin = int($z)% 2; return $coin; };
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 :)

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
    QM! Thanks so much!

    It'll take a while to digest all of your feedback, but there is one thing that leaps out:

    I couldn't figure out how to pass a nested subroutine. This is adapted from the Perl Cookbook: (and one of the beginner-gotchas that got me):

    sub outloop { #NOOB GOTCHA my $k = "string"; sub inside { return $k."cheese" } return inside( ); }
    So I thought I had to do this, without figuring out how to use references (or restructure the whole thing):
    sub outer { #APPARENTLY CORRECT my $k = "string"; local *inside = sub { return $k."cheese" }; return inside( ); }

    ry

    "I'm not afraid of Al Quaeda. I'm afraid of Al Cracker." -Chris Rock
      I couldn't figure out how to pass a nested subroutine.
      "pass" a sub? You didn't pass a sub. You defined it, then used it, then returned it's return value. If you want to pass a sub, you need a reference to it. Off the top of my head, there are 2 easy ways:

      1) Define an anonymous (no-name) sub, and assign it to a scalar ref

      my $square_this = sub { my $x = shift; return $x**2; };
      2) Take a reference to a named sub
      sub square { my $x = shift; return $x**2; } my $square_this = \&square;
      Use it like this (either version):
      my $squared = $square_this->($hypotenuse);
      Perhaps what you really meant was nested sub, in the Pascal sense. In Perl there's no such animal per se, though it can be emulated. In Perl we could talk about a limited scope sub, only available to a certain sub or collection of subs. Again, a couple of ways:

      1) Packages: define a package to limit scope

      package Square; sub square { my $x = shift; return $x**2; }
      Use it one of these ways:
      1) Inside package Square (perhaps by other routines in that package)
      my $squared = square($hypotenuse);
      2) Outside of package Square, e.g., main:
      my $squared = Square::square($hypotenuse);
      3) If you do this, it won't be limited in scope.
      Package Square is in Square.pm, the following is in main:
      use Square qw(square); my $squared = square($hypotenuse);
      The solution I think you're looking for (and tell me if I'm wrong), is a closure with an anonymous sub, that can only be seen by your named sub. For instance:
      { # closure for cubed my $square = sub { my $x = shift; return $x**2; }; sub cubed { my $z = shift; return $z*$square->($z); } } # end closure for cubed
      Now only the subs (and other code) in the closure can see $square (unless you give out a reference to it).

      -QM
      --
      Quantum Mechanics: The dreams stuff is made of