Pre-Merge Review: Maintainability

Part II · Module 27 · Chapter 27.10 · Pre-Merge Review: Maintainability

1. The Checklist, and the Two Halves of a Review

Chapter 27.7 applied the practices that make an inherited script safe to edit. This chapter turns them into a review that runs before a change is merged, so that new scripts start in the state old ones had to be brought to.

Every question has two halves. Is it there? — is strict present, is there a usage message — can be checked by a script, quickly and on every change. Is it right? — does the usage message describe what the script really does, does the subroutine's name say what it does — needs a reviewer. The mechanical half is worth automating precisely so that the reviewer's attention goes to the second.

2. The Mechanical Half as a Script

The usual tool for this in Perl is Perl::Critic. It is a CPAN distribution, not part of core Perl, and it is not installed on the machine these examples were run on — a common situation on locked-down compute farms. The checker below is deliberately small: one function per question, each answering "is it there?" and nothing more.

#!/usr/bin/perl
# maint_review.pl SCRIPT... -- the mechanical half of the maintainability
# review. Each check answers "is it there?"; a reviewer still answers "is it
# right?". Exit 1 if any script fails any check.
use strict;
use warnings;

my @CHECKS = (                      # [ name, waiver key, check ]
    [ "strict + warnings", "strict",   \&strict_and_warnings ],
    [ "no stray globals",  "globals",  \&no_stray_globals    ],
    [ "named subroutines", "subs",     \&named_subs          ],
    [ "patterns once",     "patterns", \&patterns_once       ],
    [ "usage message",     "usage",    \&usage_message       ],
    [ "documented I/O",    "doc",      \&documented_io       ],
);
# A finding can be waived only in the script itself, with a reason:
#     # review-ok: patterns -- two layouts, one per tool version (18.8)

my $failed = 0;
for my $file (@ARGV) {
    open my $fh, "<", $file or die "$file: $!\n";
    my @src = <$fh>;
    close $fh;
    my %waived = map { /^\s*#\s*review-ok:\s*(\w+)\s+--\s*(\S.*)$/ ? ($1 => $2) : () } @src;
    my (@ok, @notes);
    for my $c (@CHECKS) {
        my ($name, $key, $check) = @$c;
        my $problem = $check->(\@src, $file);
        if (defined $problem && $waived{$key}) {
            push @ok, "ok*";
            push @notes, "$name: waived -- $waived{$key}";
            next;
        }
        push @ok, defined $problem ? "NO " : "yes";
        push @notes, "$name: $problem" if defined $problem;
    }
    my $real = grep { !/: waived --/ } @notes;
    $failed++ if $real;
    printf "%-28s %s\n", $file, join("  ", @ok);
    print "    $_\n" for @notes;
}
exit($failed ? 1 : 0);

sub strict_and_warnings {
    my ($src) = @_;
    my $text = join "", @$src;
    my @missing = grep { $text !~ /^\s*use\s+$_\b/m } qw(strict warnings);
    return @missing ? "missing: use @missing" : undef;
}

sub no_stray_globals {
    my ($src, $file) = @_;
    # perl -wc reports every package variable used only once: a typo or a stray global
    my @once = grep { /used only once/ } `perl -wc "$file" 2>&1`;
    return @once ? scalar(@once) . " variable(s) used only once" : undef;
}

sub named_subs {
    my ($src) = @_;
    my $code = grep { /\S/ && !/^\s*#/ } @$src;
    my $subs = grep { /^\s*sub\s+\w+/ } @$src;
    return ($code > 15 && !$subs) ? "$code lines of code and no named subroutine" : undef;
}

sub patterns_once {
    my ($src) = @_;
    my %by_word;                         # the main literal word in each pattern
    for my $line (@$src) {
        next if $line =~ /^\s*#/;
        while ($line =~ m{(?:=~\s*m?|qr)/((?:[^/\\]|\\.)+)/}g) {
            my $pat = $1;
            my ($word) = sort { length $b <=> length $a } $pat =~ /([A-Za-z]{3,})/g;
            $by_word{$word}{$pat}++ if defined $word;
        }
    }
    my @dup = grep { keys %{ $by_word{$_} } > 1 || (values %{ $by_word{$_} })[0] > 1 } sort keys %by_word;
    return @dup ? "written more than once for: @dup" : undef;
}

sub usage_message {
    my ($src) = @_;
    my $text = join "", @$src;
    return undef unless $text =~ /\@ARGV|<>/;          # takes no arguments: nothing to explain
    return $text =~ /usage:/i ? undef : "reads its arguments but prints no usage";
}

sub documented_io {
    my ($src) = @_;
    my $head = join "", grep { /^\s*#/ } @$src[ 0 .. ($#$src < 9 ? $#$src : 9) ];
    my @missing = grep { $head !~ /\b$_\b/i } qw(usage output exit);
    return @missing ? "header does not say: @missing" : undef;
}

How each check works, and what that means for trusting it:

• strict + warnings looks for the two use lines.
• no stray globals runs perl -wc and counts "used only once" warnings — Chapter 27.7 showed this is the check that names a typo'd global.
• named subroutines flags a script with more than fifteen lines of code and no named subroutine.
• patterns once groups every literal pattern by its main word and flags a word that appears in two different patterns, or one pattern written twice.
• usage message flags a script that reads its arguments but never prints the word usage:.
• documented I/O looks for usage, output and exit in the comment block at the top.

Each check is a heuristic, and section 6 measures where that matters.

3. Running It on This Module's Own Scripts

The scripts below are all ones this module published, from the inherited original to the version built on the shared module, plus the two parsers from Chapter 27.9:

#!/bin/bash
printf '%-28s %s\n' "" "strict globals subs pats usage doc"
perl maint_review.pl old_summary.pl step3_lexical.pl step6_named_patterns.pl \
    step7_shared_module.pl parse_report_submitted.pl parse_report_reviewed.pl
echo "maint_review.pl exit status: $?"

                             strict globals subs pats usage doc
old_summary.pl               NO   NO   NO   NO   NO   NO 
    strict + warnings: missing: use strict warnings
    no stray globals: 2 variable(s) used only once
    named subroutines: 21 lines of code and no named subroutine
    patterns once: written more than once for: ERROR
    usage message: reads its arguments but prints no usage
    documented I/O: header does not say: usage output exit
step3_lexical.pl             NO   yes  yes  NO   NO   NO 
    strict + warnings: missing: use warnings
    patterns once: written more than once for: ERROR
    usage message: reads its arguments but prints no usage
    documented I/O: header does not say: usage exit
step6_named_patterns.pl      yes  yes  yes  NO   yes  yes
    patterns once: written more than once for: ERROR
step7_shared_module.pl       yes  yes  yes  yes  yes  yes
parse_report_submitted.pl    yes  yes  yes  yes  NO   NO 
    usage message: reads its arguments but prints no usage
    documented I/O: header does not say: usage output exit
parse_report_reviewed.pl     yes  yes  NO   NO   NO   NO 
    named subroutines: 31 lines of code and no named subroutine
    patterns once: written more than once for: VIOLATED
    usage message: reads its arguments but prints no usage
    documented I/O: header does not say: usage
maint_review.pl exit status: 1

The table reads like the history of Chapters 27.6 and 27.7:

old_summary.pl fails all six. That is the inherited script as it arrived — no strict, two variables used once (the typo), no subroutines, two different patterns for ERROR, no usage, no header.

step3_lexical.pl passes two. strict and lexical variables fixed the globals and the structure; warnings, usage and documentation came later.

step6_named_patterns.pl passes five, and is flagged for ERROR. That flag is correct, and it is the same inconsistency Chapter 27.7 deliberately left visible: $ERROR_WORD and $ERROR_LINE are two different definitions of an error, sitting on adjacent lines. The checker found it on its own.

step7_shared_module.pl passes all six.

The 27.9 parsers. The submitted one is short and has strict, but no usage and no header. The reviewed one is more correct, as Chapter 27.9 measured, and fails four checks — robustness and maintainability are different properties, and a change can improve one while leaving the other behind.

4. A Finding That Is a Decision

One of the reviewed parser's findings is not a defect. patterns once flagged the word VIOLATED, because the parser defines two slack patterns. Chapter 27.9 added the second on purpose: one pattern per tool version in use. A checker cannot know that; a reviewer can.

The answer is not to weaken the check, and not to ignore its output. It is a waiver written in the script itself, with a reason:

# review-ok: patterns -- one slack layout per tool version in use (18.8, 27.9)

The checker honours a waiver only in that form — a known check name, two dashes, and a reason — and shows the finding as ok* with the reason printed beneath it. The reason is what makes it a decision rather than a suppression. A waiver with no reason is a finding someone did not want to see, and the next engineer cannot tell which one they are looking at.

5. Making the 27.9 Parser Pass, Behaviour Unchanged

Here is the reviewed parser after the maintainability review: a header that states the usage, the output and every exit status; a usage message when it is run with no input; the reading and the reporting split into two named subroutines; and the waiver.

#!/usr/bin/perl
# parse_report.pl -- summarise the paths in synthetic timing reports (26.4 format).
#   usage:  parse_report.pl REPORT...      (or a report on standard input)
#   output: one line per corner: "CORNER paths=N worst=SLACK STATE" (stdout)
#   exit:   0 every report complete and understood; 2 anything incomplete,
#           unreadable, or no report found (message on stderr)
# review-ok: patterns -- one slack layout per tool version in use (18.8, 27.9)
use strict;
use warnings;

my $SLACK_V1 = qr/^\s+slack \((MET|VIOLATED)\)\s+(-?\d+\.\d+)\s*$/;   # status, then value
my $SLACK_V2 = qr/^\s+slack (-?\d+\.\d+) \((MET|VIOLATED)\)\s*$/;     # value, then status

if (!@ARGV && -t STDIN) {
    print STDERR "usage: parse_report.pl REPORT...\n";
    exit 2;
}

my @reports = read_reports();
unless (@reports) {
    print STDERR "parse_report: no 'Corner :' line -- not a timing report\n";
    exit 2;
}
my $bad = grep { report_line($_) ne "OK" } @reports;
exit($bad ? 2 : 0);

# One record per "Corner :" section, closed by "End of report".
sub read_reports {
    my (@reports, $cur);
    while (my $line = <>) {
        if ($line =~ /^Corner\s*:\s*(\S+)/) {
            $cur = { corner => $1, slack => [], complete => 0, unknown => 0 };
            push @reports, $cur;
            next;
        }
        next unless $cur;
        if    ($line =~ $SLACK_V1)          { push @{ $cur->{slack} }, $2 }
        elsif ($line =~ $SLACK_V2)          { push @{ $cur->{slack} }, $1 }
        elsif ($line =~ /^\s+slack\b/)      { $cur->{unknown}++ }
        elsif ($line =~ /^End of report$/)  { $cur->{complete} = 1; undef $cur }
    }
    return @reports;
}

# Print one corner's line; return its state.
sub report_line {
    my ($r) = @_;
    my ($worst) = sort { $a <=> $b } @{ $r->{slack} };
    my $state = !$r->{complete} ? "INCOMPLETE"
              : $r->{unknown}   ? "UNREADABLE_SLACK_LINES=$r->{unknown}"
              :                   "OK";
    printf "%s paths=%d worst=%s %s\n", $r->{corner}, scalar @{ $r->{slack} }, $worst // "n/a", $state;
    return $state;
}

A maintainability change must not change behaviour — Chapter 27.6's rule. So the driver checks both: the review, and the output on every one of Chapter 27.9's seven inputs, produced again by its make_inputs.sh with the teaching mock:

#!/bin/bash
perl maint_review.pl parse_report_final.pl
echo "maint_review.pl exit status: $?"
echo
bash make_inputs.sh > /dev/null
same=0
for f in in/*; do
    a=$(perl parse_report_reviewed.pl "$f" 2>&1; echo "exit $?")
    b=$(perl parse_report_final.pl   "$f" 2>&1; echo "exit $?")
    if [ "$a" = "$b" ]; then same=$((same + 1)); else echo "  CHANGED on $f"; fi
done
echo "$same of $(ls in | wc -l | tr -d ' ') inputs: same stdout, stderr and exit status as before the review"

parse_report_final.pl        yes  yes  yes  ok*  yes  yes
    patterns once: waived -- one slack layout per tool version in use (18.8, 27.9)
maint_review.pl exit status: 0

7 of 7 inputs: same stdout, stderr and exit status as before the review

Five checks pass, one is waived with its reason shown, and the checker exits 0. On all seven inputs the parser's output, messages and exit status are exactly what they were before the review. The script got easier to change without changing what it does.

6. What the Script Cannot See

A mechanical review that passes is easy to over-trust, so it is worth measuring a script it gets wrong. The one below was written for this chapter to defeat the checker. It has every surface feature the checks look for, and switches strict back off one line later:

#!/usr/bin/perl
# blind_spot.pl -- count the error reports in simulation logs.
#   usage:  blind_spot.pl LOG...
#   output: one count per log (stdout)
#   exit:   0; 2 for a usage error
use strict;
use warnings;
no strict 'vars';                 # ...and strict is off again

unless (@ARGV) { print STDERR "usage: blind_spot.pl LOG...\n"; exit 2 }

$keyword = "ERROR";               # a global, used twice, so -wc stays quiet
for my $log (@ARGV) {
    print "$log ", count($log), "\n";
}

sub count {
    my ($log) = @_;
    open my $fh, "<", $log or die "$log: $!\n";
    my $n = grep { /$keyword/ } <$fh>;     # the same pattern as /^ERROR: /, defined
    return $n;                             # differently, through a variable
}

#!/bin/bash
perl maint_review.pl blind_spot.pl
echo "maint_review.pl exit status: $?"
echo "--- and what it reports on two real logs"
perl blind_spot.pl inputs/icarus_info_mentions_error.log inputs/icarus_mismatch.log

blind_spot.pl                yes  yes  yes  yes  yes  yes
maint_review.pl exit status: 0
--- and what it reports on two real logs
inputs/icarus_info_mentions_error.log 2
inputs/icarus_mismatch.log 1

All six checks pass, and the script is wrong. It reports two errors in a log from a passing run — the unanchored-pattern defect from Chapter 27.8, counting the INFO lines that mention ERROR. Three things defeated the checker:

• no strict 'vars' one line after use strict. The check looks for the use line; it does not ask whether strict stays on.
• A global used twice. perl -wc reports only variables used once; $keyword, set and read, is invisible to it.
• A pattern built from a variable. /$keyword/ has no literal word for the checker to compare with ERROR, so two different meanings of "error" pass as one pattern.

None of these is exotic, and each could be added to the checker. But every rule added to a checker has its own way around it. The checker's real value is that it is fast, cheap and never tired — and that its passes are necessary conditions for a maintainable script, never sufficient ones.

7. The Human Half

After the script has run, the reviewer's questions are the ones only a person can answer. Six, matching the checklist:

• Strict and warnings: are they on for the whole file, with no no strict or no warnings that is not explained in a comment?
• Globals: is every remaining package variable (our) there for a reason — an export list, a version — rather than as a shortcut?
• Subroutines: does each name say what the subroutine does, and does each one do one thing? A subroutine called process that reads, parses and prints passes the check and fails the question.
• Patterns: does each named pattern say, in its comment, what real output it matches (Chapter 27.5's table)? Is every waiver's reason still true?
• Usage: does the usage message match the real arguments, and does a wrong invocation exit with the status the header documents?
• Documentation: does the header describe this version of the script, or the one from three changes ago?

The checker makes sure the reviewer never has to spend attention on whether something exists. The reviewer's job is whether it is true.

8. Exercises

1. Close one blind spot. Extend maint_review.pl so that blind_spot.pl fails the strict check because of no strict 'vars', while parse_report_final.pl still passes. Then write a new specimen that gets around your extension, and decide whether that one is worth closing too.

2. A waiver without a reason. Write # review-ok: patterns with no reason into a copy of parse_report_final.pl. What does the checker do, and is that the behaviour you want? Change it if not, and justify the choice.

3. Review step 6's decision. The checker flagged step6_named_patterns.pl for its two ERROR patterns. Write the reviewer's comment on that finding: either the waiver line with its reason, or the requested change and why. Use Chapter 27.6's measurement as evidence.

4. Run it on your flow. Run maint_review.pl over every Perl script in a flow you have access to. Sort the scripts by number of failed checks and pick the worst three. For each, estimate what Chapter 27.7's steps would cost and what the first step should be.

5. Necessary, not sufficient. Write the shortest script you can that passes all six checks and is unmaintainable in a way the human-half questions in section 7 would catch. Which question catches it?

9. What To Remember

• Six questions: strict and warnings, no stray globals, named subroutines, patterns once, usage, documented I/O.
• Every question has a mechanical half ("is it there?") and a human half ("is it right?"). Automate the first so a reviewer can spend time on the second.
• Perl::Critic is CPAN and was not installed here; a small checker with one function per question still catches the common cases.
• Measured on this module's scripts: the inherited original failed 6 of 6; the shared-module version passed 6 of 6.
• The checker found Chapter 27.7's open decision on its own — two ERROR patterns side by side.
• A finding that is a deliberate decision gets a waiver in the script, with a reason. No reason, no waiver.
• Maintainability changes must not change behaviour. Measured: the reviewed parser was revised to pass, with identical stdout, stderr and exit status on all 7 inputs.
• A clean mechanical review is necessary, not sufficient. Measured: a script passed 6 of 6 and still counted 2 errors in a passing log.
• The reviewer's questions are about truth: does each name, message, comment and waiver still describe the code?

Chapter 27.11 is the last review: whether a tool behaves the way its callers rely on — re-runnable, exit status matching its verdict, data and diagnostics on separate streams, ordered output, and writing only where it should.