note
GrandFather
<p>Don't initialise all your variables in a great bolus at the top of your code. You wipe out half the advantage of using strictures if you do so. Instead declare and initialise variables to have as small a scope as makes sense. Always initialise variables with their first value when they are declared unless the declaration must be outside a loop.</p>
<c>
my $i = 0;
foreach (@ARGV) {
if ($i == 0) {
print "$_";
$i = 1;
}
else {
print ", $_";
}
}
</c>
<p>is much better written:</p>
<c>
print join ', ', @ARGV;
</c>
<p>New acts and scenes are better managed by:</p>
<c>
my $new_act;
my $new_scene;
while (<$playIn>) {
my $line = $_;
...
# Print the act heading if we haven't yet.
if ($new_act) {
print "$new_act\n";
$new_act = undef;
}
# Print the scene headin if we haven't yet.
if ($new_scene) {
print "$new_scene\n";
$new_scene = undef;
}
...
# Step 1.6: We should also print act headings.
elsif ($line =~ m/.*<h3>ACT \w+.*/) {
$new_act = $line;
}
# Step 1.7: We should also print scene headings.
elsif ($line =~ m/.*<h3>SCENE \w+.*/) {
$new_scene = $line;
}
}
</c>
<p>Don't use manifest constants ("true" and "false") for boolean values. They are prone to error and obscure the flow of the code by adding code. <c>if ($new_scene)</c> is easy to read an understand - anything more is harder to read, understand and get right.</p>
<p>Use three parameter [doc://open] and lexical file handles:</p>
<c>
open my $playIn, '<', $mit_shakes or die "Could not open '$mit_shakes': $!\n";
</c>
<p>Note that your open (<c>open PLAY, $mit_shakes || die "Could not open file: $!\n";</c>) is broken. Replace <c>||</c> with <c>or</c> for it to work as you expect.</p>
<div class="pmsig"><div class="pmsig-461912">
True laziness is hard work
</div></div>
1009211
1009211