For the amusement of all camels and llamas present, here's a code-review of
Perlwar done by Uri. For the record, he sent it my way barely a few hours
after the initial release, which speaks volumes about the man's alacrity, and
my weakness for procrastination.
Joy,
`/anick
---------- Forwarded Message ----------
<off list but you can forward this to fwp if you want>
amusing. i browsed the code and it doesn't look too bad. i especially
analyzed the code of run_slot as it seems to be the heart of the
program. here are is some code review of it:
sub runSlot
{
my ( $self, $slotId ) = @_;
return unless $self->{theArray}[ $slotId ];
you have many common subexpressions (hash accesses) that slow things
down and clutter up the code. use variables to store lower level stuff
that you will need again later.
my $theArray = $self->{theArray} ;
my $curr_slot = $theArray->[ $slotId ];
return unless $curr_slot ;
you don't need the %slot hash because you have a ref to it.
********
my %slot = %{$self->{theArray}[ $slotId ]};
return if $slot{freshly_copied} or not $slot{code};
return if $curr_slot->{freshly_copied} or ! $curr_slot->{code};
a good rule of thumb is to use !, &&, || in expressions and not, and, or
where you do flow control (as in open or die).
$self->log( "cell $slotId: agent owned by $slot{owner}" );
my @Array = map $_->{code}, @{$self->{theArray}}[
$slotId..(@{$self->{theArray}}-1), 0..($slotId-1) ];
you do that big array slice before you do the following test. in the
test you access $Array[0] but tha could also be theArray[$slot_id]{code}
or just even $curr_slot->{code}. then you can test first and only copy
the array when you know the slot has legal code
# exceed permited size?
if( length > $self->{conf}{snippetMaxLength} )
that takes length of $_ and not of the code.
{
$self->log( "\tagent crashed: is ".length($Array[0])." chars, exceeds
max permitted size $self->{conf}{snippetMaxSize}" ); $self->{theArray}[
$slotId ] = {};
return;
}
your indent style is not the most common one which is to put { on the
end of lines of code and to indent the code by one level. this is harder
to read in mnay people's opinion. for more on this and coding styles,
get damian conway's new book, perl best practices.
the above could become:
my $code_len = length( $curr_slot->{code} ) ;
my $max_snippet = $self->{conf}{snippetMaxLength} ;
if ( $code_len > $max_snippet ) {
$self->log( <<LOG ) ;
\tagent crashed: is $code_len." chars, exceeds max permitted size
$max_snippet LOG
$theArray->[ $slotId ] = {};
return ;
}
$self->log( "\texecuting.." );
my( $result, $error );
( $result, $error, @Array ) = $self->execute( @Array );
you can merge those like this:
( my( $result, $error ), @Array ) = $self->execute( @Array );
some may not like that. i don't mind as i like to declare when things
are first used if possible.
$self->{theArray}[$slotId]{code} = $Array[0];
$curr_slot->{code} = $Array[0];
again, you check an error that would wipe stuff out after you did useful
work. do this check just after the call to execute.
if( $error ) {
$self->log( "\tagent crashed: $error" );
$self->{theArray}[$slotId] = {};
return;
}
***********
( my( $result, $error ), @Array ) = $self->execute( @Array );
if ( $error ) {
$self->log( "\tagent crashed: $error" );
$theArray->[$slotId] = {};
return;
}
my $output = $result;
$output = substr( $output, 0, 24 ).".." if length $output > 25;
$output =~ s#\n#\\n#g;
$self->log( "\tagent returned: $output" );
i notice plenty of redundant code in the next sections. i factored out
the 3 simpler return op codes and used a dispatch table to handle their
unique logic after i do the common logic
declare this table outside a sub as it is static
my %op_to_code = (
'!' => \&nuke_operation,
'^' => \&p0wn_operation,
'~' => \&alter_operation,
) ;
then in the code do a single regex, the common code and dispatch on the
opcode
if( $result =~ /^([!^~])(-?\d*)$/ ) {
by the way, is '!-' (or any other op) allowed? you don't handle that
special case of - and no digits. this regex will disallow just -:
if( $result =~ /^([!^~])(-?\d+)?$/ ) {
my $abs_pos = $self->relative_to_absolute_position( $slotId, $2 || 0 );
return if $abs_pos == -1;
my $slot = $theArray->[ $abs_pos ] ;
unless ( $slot ) {
$self->log( "\tno agent found at cell $abs_pos" );
return ;
}
my $op_code_ref = $op_to_code{ $1 } ;
$self->$op_code_ref( $theArray, $abs_pos, $slot ) ;
return ;
}
if( $result =~ /^(-?\d*):(-?\d*)$/ ) { # 212:213
$self->copy_operation( $theArray, $1, $2 ) ;
return ;
}
$self->log( "\tno-op" );
}
note that that also removed all the elsif's which are nasty. when you
see a series of them, think dispatch table or some other way to do it.
sub nuke_operation {
my( $self, $theArray, $abs_pos, $slot ) = @_ ;
$theArray->[ $abs_pos ] = { };
$self->log( "\tagent in cell $abs_pos destroyed" );
return ;
}
sub p0wn_operation {
my( $self, $theArray, $abs_pos ) = @_ ;
my $slot = $theArray->[$abs_pos] ;
# unless( $slot and $slot->{code} ) {
# $self->log( "\tno agent to p0wn in cell $abs_pos" );
# return;
# }
# this seems to need $slotID as well. a simple fix is to store the
# current slot id into the object when you know it at the top level. i
# will assume this for now
$self->log( "\tagent in cell $abs_pos p0wned" );
$slot->{owner} = $theArray->[$self->{slotId}]{owner};
# if ( $theArray->[ $abs_pos ] ) {
# $theArray->[ $abs_pos ] = { };
# $self->log( "\tagent in cell $abs_pos destroyed" );
# return ;
# }
#
# why not absolutely kill the slot? why check for it first? you only
# check it for logging but you ALREADY checked if this slot is filled
# just above! note how the next log message and the one inside the
# unless are bascially the same
# $self->log( "\tno agent found at cell $pos" );
so just destroy it at this point
$theArray->[ $abs_pos ] = { };
$self->log( "\tagent in cell $abs_pos destroyed" );
return ;
}
sub alter_operation { # pick a better name!
my( $self, $theArray, $abs_pos ) = @_ ;
# again, this needs relative slot position and its code. assume it is in
# the object or pass it in (note that all these subs in the dispatch
# table must be passed the same args.
$theArray->[$abs_pos]{code} = $self->[Array][$self->{rel_pos}];
$self->log( "\tcode of agent in cell $abs_pos altered" );
}
see how the three operation functions are very short and clear now. this
last one is longer than i want to hack up but i expect you can
clean it up like i did with the others.
{
# big problem here. we don't want a newly copied guy to be effective
# during the same turn
my $src_pos += $self->relative_to_absolute_position( $slotId, $1 );
my $dest_pos += $self->relative_to_absolute_position( $slotId, $2 );
return if $src_pos == -1 or $dest_pos == -1;
if( $self->{theArray}[$dest_pos]{owner} and
$self->{theArray}[$dest_pos]{owner} ne
$self->{theArray}[$slotId]{owner} ) {
$self->log( "\tagent in cell $dest_pos already owned by
$self->{theArray}[$dest_pos]{owner}" ); return;
}
$self->log( "\tagent of cell $src_pos copied into cell $dest_pos" );
$self->{theArray}[$dest_pos] = \%{$self->{theArray}[$src_pos]};
$self->{theArray}[$dest_pos]{freshly_copied} = 1 ;
thanx,
uri
--
Uri Guttman ------ [EMAIL PROTECTED] -------- http://www.stemsystems.com
--Perl Consulting, Stem Development, Systems Architecture, Design and Coding-
Search or Offer Perl Jobs ---------------------------- http://jobs.perl.org
-------------------------------------------------------
