in reply to Another perl battleship
# # 5x5 map grid for each player # Cruiser = * # Carrier = @ # Submarine = ~ # Ocean/Empty Space = .
Your grid is actually 10x5.
# Stats trackers my @p1Attacks; my @p2Attacks;
The variable @p2Attacks is never used anywhere so it could be removed.
# Ships - surely there is a better way to do this my %p1cruiser = ( 'hp' => '2', 'size' => '3', 'ap' => '1', 'loc' => '' +, 'sym' => '*', 'mc' => 0 ); my %p1carrier = ( 'hp' => '3', 'size' => '5', 'ap' => '2', 'loc' => '' +, 'sym' => '@', 'mc' => 0 ); my %p1subm = ( 'hp' => '1', 'size' => '2', 'ap' => '3', 'loc' => '' +, 'sym' => '~', 'mc' => 0 ); my %p1ships = ( 'cru' => \%p1cruiser, 'car' => \%p1carrier, 'subm' => +\%p1subm ); my %p2cruiser = ( 'hp' => '2', 'size' => '3', 'ap' => '1', 'loc' => '' +, 'sym' => '*', 'mc' => 0 ); my %p2carrier = ( 'hp' => '3', 'size' => '5', 'ap' => '2', 'loc' => '' +, 'sym' => '@', 'mc' => 0 ); my %p2subm = ( 'hp' => '1', 'size' => '2', 'ap' => '3', 'loc' => '' +, 'sym' => '~', 'mc' => 0 ); my %p2ships = ( 'cru' => \%p2cruiser, 'car' => \%p2carrier, 'subm' => +\%p2subm );
The hashes %p1cruiser, %p1carrier, %p1subm, %p2cruiser, %p2carrier and %p2subm aren't really needed:
# Ships - surely there is a better way to do this my %p1ships = ( cru => { hp => 2, size => 3, ap => 1, loc => '', sym => '*', mc = +> 0 }, car => { hp => 3, size => 5, ap => 2, loc => '', sym => '@', mc = +> 0 }, subm => { hp => 1, size => 2, ap => 3, loc => '', sym => '~', mc = +> 0 }, ); my %p2ships = ( cru => { hp => 2, size => 3, ap => 1, loc => '', sym => '*', mc = +> 0 }, car => { hp => 3, size => 5, ap => 2, loc => '', sym => '@', mc = +> 0 }, subm => { hp => 1, size => 2, ap => 3, loc => '', sym => '~', mc = +> 0 }, );
# Get in use tiles for ship hashes foreach my $ship ( keys %p1ships ) { if ( ! $p1ships{$ship} ) { next; } my $shipRef = $p1ships{$ship}; my $location = ${$shipRef}{loc}; my @inUseTiles = split(",", $location); foreach my $iut ( @inUseTiles ) { push(@p1usedTiles, $iut); } }
push is a list operator so you don't need a foreach loop. And you don't really need all those intermediary variables.
# Get in use tiles for ship hashes foreach my $ship ( keys %p1ships ) { next unless $p1ships{ $ship }; push @p1usedTiles, split /,/, $p1ships{ $ship }{ loc }; }
sub checkLocation { # Given a set of coordinates, determine if they are already occupi +ed my $taken = 0; my $coordinates = shift; if ( $coordinates !~ /^[0-9]*,[0-9]*$/ && $coordinates !~ /^[0-9]* +,[0-9]*,[0-9]*$/ && $coordinates !~ /^[0-9]*,[0-9]*,[0-9]*,[0-9]*,[0- +9]*$/ ) { print "These coordinates look incorrect, you shouldnt see this + error...\n"; $taken = $taken + 1; } my @coors = split(/,/, $coordinates); foreach my $coor ( @coors ) { if ( $p1map{$coor} ne "." ) { print "coordinate $coor contains $p1map{$coor}\n"; $taken = $taken + 1; } } if ( $taken >= 1 ) { return 1; } else { return 0; } }
You are checking $coordinates for either one, two or four commas, with optional numbers. Perhaps you should change [0-9]* to [0-9]+.
$taken = $taken + 1 could be shortened to $taken += 1 or even ++$taken.
The return code could be shortened to return $taken >= 1 ? 1 : 0 or even return $taken >= 1.
# Get random ship key and href my @availShips; foreach my $key ( keys %p2ships ) { if ( ! defined $p2ships{$key} ) { next; } else { push(@availShips,$key); } }
That can be shortened a bit:
# Get random ship key and href my @availShips = grep defined( $p2ships{ $_ } ), keys %p2ships;
# Make sure AI doesn't try to 'move' if it has no available moves +left print "Checking available AI moves\n"; my @availMovers; foreach my $key ( keys %p2ships ) { my $shipRef = $p2ships{$key}; if ( ! defined $p2ships{$key} ) { next; } elsif ( ${$shipRef}{mc} == 1 ) { next; } else { push(@availMovers, $key); } }
Same as with above:
# Make sure AI doesn't try to 'move' if it has no available moves +left print "Checking available AI moves\n"; my @availMovers = grep defined( $p2ships{ $_ } ) && $p2ships{ $_ } +{ mc } != 1, keys %p2ships;
# Menu loop while () { my $count = 0; if ( $count == 0 ) { &printMenu; } ... $count++; }
The $count variable should be defined outside the loop if you want it to work correctly:
# Menu loop my $count = 0; while () { if ( $count == 0 ) {
&printMenu should be printMenu().
my @opponentRemaining; foreach my $key ( keys %p2ships ) { if ( defined $p2ships{$key} ) { push(@opponentRemaining, $key) } }
Same as with above:
my @opponentRemaining = grep defined( $p2ships{ $_ } ), ke +ys %p2ships;
my @validInputs; foreach my $key ( keys %p1ships ) { my $shipHref = $p1ships{$key}; my $moveCounter = ${$shipHref}{mc}; if ( ! defined $p1ships{$key} ) { next; } elsif ( $moveCounter == 1 ) { next; } else { push(@validInputs,$key); } }
Same as with above:
my @validInputs = grep defined( $p1ships{ $_ } ) && $p +1ships{ $_ }{ mc } != 1, keys %p1ships;
Dereferencing like ${$shipHref}{mc} are usualy written as $shipHref->{mc}.
|
|---|
| Replies are listed 'Best First'. | |
|---|---|
|
Re^2: Another perl battleship
by hippo (Archbishop) on Jan 09, 2019 at 09:14 UTC | |
|
Re^2: Another perl battleship
by spesk (Novice) on Jan 09, 2019 at 14:58 UTC |