in reply to Shuffling cards

Unfortunately I don't have the time to see what this is really about and to delve into your code in its entirety. I have a few general remarks that hopefully will help you anyway...

Update: eventually I checked the rules and I provide some minimal but complete working example here: 433664.

#!/usr/bin/perl # Shuffles cards
Always -yes: always!-
use strict; use warnings;
in all but most trivial code (e.g. one-liners). This does not address your specific issue, but applies to any issue...
@pack = (1, 2, 3, 4, 5, 6, 7, 8);
As a side note, since you use .. below, I can't see why you don't use it here too...
sub b { push(@pack, shift @pack); }
Hmmm, while occasionally it can be handy to have a sub manipulating some "global" data, parameter passing is generally a much better solution. It's what a function is for.

I suggest you read something about @_, e.g. perlsub.

sub i { my ($a, $b, $c, $d, $e, $f, $g, $h) = @pack; @pack = ($e, $a, $f, $b, $g, $c, $h, $d); } sub o { my ($a, $b, $c, $d, $e, $f, $g, $h) = @pack; @pack = ($a, $e, $b, $f, $c, $g, $d, $h); }
Ditto as above. But then you may also use slicing instead. In that case you may also "generate" the subscripts rather than hardcoding them, but I agree that in this case you wouldn't earn much and probably hardcoding them is the best choice...

Also, it is always recommended to avoid using $a and $b as general purpose variables. This can lead to confusion and gotchas (see perlvar).

sub parse { my ($string) = @_; print "\nParse $string: "; foreach $offset (0..(length($string)-1)) { $char = substr($string, $offset, 1);
I think that most people here would write
for (string =~ /./g) { # ...
or
for (split //, $string) { # ...
instead. As a side note I would also have (a local()) $_ instead of $string and so I'd have
for (/./g) { # ...
or
for (split //) { # ...
respectively.
if ($char =~ /b|i|o/) { print "bio "; &{$char}; }
Ouch!
elsif ($char =~ /\d/) { print "digit "; # foreach (1..$char) { &parse(substr($string, $offset+1)); +}
Ditto as above. Also, apart that the line is commented out, do you really want to have 1..$char?!?

Update: never mind, there's nothing wrong a priori with 1..$char above, only you now have foreach (1..$char) but you use $offset as if it were the loop iterator, in the loop.

Well, I couldn't go past this point in your code in detail, but fundamentally it seems to me that the same cmts as above apply. I will look into the rules and maybe provide some minimal sample code as time permits. (But then I'm sure others will do!)

Replies are listed 'Best First'.
Re^2: Shuffling cards
by amnesty_puppy (Novice) on Feb 22, 2005 at 15:17 UTC

    Wow, thanks for the review - pointed out a lot of things I need to look over. Some of the bits I did because it's a timed exercise (working on global var + symrefs) but seems I made a lot of time-wasting mistakes as well (and just bad mistakes :D).

    As to 1..$char? iterate $char times, what's wrong with that?

    Anyway.. <runs off to improve code>

      Wow, thanks for the review - pointed out a lot of things I need to look over. Some of the bits I did because it's a timed exercise (working on global var + symrefs) but seems I made a lot of time-wasting mistakes as well (and just bad mistakes :D).
      Incidentally using a hash, e.g. a dispatch table takes only a few more efforts than symrefs.
      As to 1..$char? iterate $char times, what's wrong with that?
      You're right, my mistake, I just corrected the original post.