kd

So lucky for me a client decided to pay me to refactor some of their very old code. Refactoring can be fun, but if you have a 20 year old business critical codebase where the team has forgotten or don't know how stuff works and it absolutely has to not break, then you have some challenges and quite a lot of potential for loss of face.

This particular job was to refactor a single large, excessively complex subroutine into something that was testable and that a relatively naive programmer could reason about. And there were no tests.

tl;dr: this blog post is relatively involved, but scroll down to the bottom to see some neat abuse of git as a data analysis assistant.

That was stage 1, covered in this article. Stage 2, not yet completed is to move the whole thing into a proper object oriented style rather than something that's a big hash reference shoved into a $self. The general structure of the offending subroutine is below:

sub do_thing {
    my ($self, %args) = @_;

    my $thing = 0;
    if ($args{this} && $self->some_method($args{that} || $args{other}) {
        $thing = 1;
        if ($args{this} > 999 || $args{that} < 3) {
            $self->{key} = { @some_complex_statement };
        }
        elsif ( $args{other} < 222 ) {
            $self->{key} =
                { @some_similarly_syntactically_complex_statement
                  # this statement may or may not have been syntactically 
                  # identical to the last one
                 };
    }
    else {
        # you get the idea now
    }

   if ( [some other conditional] ) {
       # nest some more stuff in here
   }
  # repeat a few more times with arbitrary depth and arbitrary
  # numbers of complex looking statements

}

The first thing I did was to satisfy myself that the problem was actually only with a single subroutine. Which fortunately it was.

The second thing I did was create a test based on some real data and then eyeballed the code coverage with Devel::NYTProf. This satisfied me that there was no dead code in my test.

The next thing I did was factor out all complex conditionals into methods that returned true or false, and removed them from the over-long package they were in with the help of Role::Tiny. That simplified things a bit, but spotting and analysis of the %self->{$key} = { @some_complex_statement } parts was still necessary. They were all hashref assignments with subtly different payloads, and if in fact they were syntactically identical, needed to be changed to a method call: $self->frobincate(@args).

So enter PPI for static analysis of perl code syntax. In fact I used my module PPIx::Refactor for this, and the script I used made it into the distribution as an example. Here's the same example with annotations:

First I found the subroutine of interest.

use PPIx::Refactor;
my $pxr1 = PPIx::Refactor->new(file => 'lib/SomePackage.pm',
                               ppi_find => sub {
                                   my ($elem, $doc) = @_;
                                   return 0 unless $elem->class eq 'PPI::Statement::Sub';
                                   return 0 unless $elem->first_token->snext_sibling eq 'my_method';
                                   return 1;
                               },);

my $sub_finds = $pxr1->finds;
die "Found more than one method of the same name " unless @$sub_finds == 1;

Once I found the parse-tree of that subroutine after some messing around with App::PPI::Dumper I decided I needed to find all PPI::Statements inside that subroutine

my $sub = $sub_finds->[0];
my $pxr2 = $pxr1->new(element => $sub,
                      ppi_find => sub {
                          my ($elem, $doc) = @_;
                          return 1 if $elem->class eq 'PPI::Statement';
                          return 0;
                      });

my $found = $pxr2->finds;

At this point some of the PPI::Statements in $found were what I needed, and some were not. After some debugging I determined that the second statement was an example of one of the suspects. Flattening the syntax tree was pretty easy:

my $candidate = join ' ', map { $_->class } grep { $_->significant} $found->[1]->tokens;

Given this is just one line of perl, factoring this into a more flexible PPI based parser would be relatively straightforward should the occasion demand (for example we have different length lists as references in the suspect statement), but fortunately I didn't need to.

To exclude false negatives I also collected up PPI::Statements that did not match for further visual examination. In the code I was working on I had some postfix conditionals ( i.e. do_stuff(@args) if $conditional) attached to the assignment, so I had to adjust $candidate appropriately, but the simple condition is given below:

my (@hit, @miss);
foreach my $statement (@$found) {
    my $syntax = join " ", map {$_->class } grep { $_->significant} $statement->tokens;
        if ($syntax eq $candidate) {
                push @hit, $statement;
        }
        else {
            push @miss, $statement;
        }
}

Finally once I had confirmed that @hit and @miss were accurate, I needed to collect up all @hits as text for further analysis:

use Path::Tiny;
path('/tmp/ppi_finds.pl')->spew(join  ";\n\n", @hit);

At this point I had a double newline separated set of statements. I created a new temporary git repository, normalised the whitespace for each statement (with emacs), and created one commit for each statement, with the statement being in a single statement.pl file.

I used an emacs macro for each commit, and a little piece of shell to actually make the commit:

git diff --stat | ack --nopager statement.pl &&  git commit -a -m '2' || echo no

Where the 2 indicates statement number two. If the two commits were identical the shell would output no, I'd go back into the editor, add some whitespace, recommit, then remove the whitespace and git commit --amend.

So at the end of this process I had as many commits as candidate statements in the code. The output of git log -p showed me if there were identical statements next to each other but what I wanted was to know which commits were the same as each other. Enter another piece of perl:

#!/usr/bin/env perl;
use warnings;
use strict;
use Algorithm::Combinatorics qw/combinations/;
use IPC::System::Simple qw/system capture/;
my @shas = capture "git log --format='%s %h'";
my %shas;
foreach  (@shas) {
    chomp;
    my ($val, $k) = split ' ', $_;
        $shas{$k} = $val;
    }
    my $iter = combinations([keys %shas], 2);
    while (my $c = $iter->next) {
        my $cmd = "git diff --stat "
                  . join (' ' , @$c )
                  . " | grep -q 'list.pl' || echo \""
                  . sprintf('%-3s%-3s', $shas{$c->[0]},  $shas{$c->[1]}) . " identical\" ";
    system $cmd;
}

So what this little script does is compare every commit against every other commit, and outputs the two commit messages on a single in the case that they're identical. In this case I had 18 statements - 153 possible combinations. Of these, 32 pairs of commits were identical to each other, and once I reconciled the output, I determined that there were only four different assignment statements. So lots of duplication, and I factored that out into an abstract method, and four methods that invoked the abstract method with specific parameters.

I could tell that by the time I'd finished I'd done the right thing, because:

1. The code was much easier to reason about even if I personally was still unclear on what it was actually trying to achieve.

2. I had four points of entry for mutating the contents of $self rather than eighteen.

3. Excluding tests the commit resulted in reducing the size of the affected code by 20% (including a whole bunch of code in the class that I hadn't touched at all).

4. There were actually tests, and every conditional statement and assignment had its own test.

This code was not egregiously bad, according to estimates I've made with the very useful Perl::Metrics::Simple CPAN module, I've worked with code that's ten times worse. For code like this interfacing with multiple external APIs, with a busy developer team who have worse things to worry about in their issue tracker it's easy to avoid fiddly jobs like this by putting them in the too hard basket -- especially if you accidentally break things while improving the code your reputation can take a big hit.

Hopefully next time I write something on refactoring it will be about factoring out nasty nested conditional statements into a flat structure that's much easier to reason about and test.