Resurrecting an old Perl codebase part 3

This is part three of an exploration of some ideas around how I approach picking up a legacy Perl codebase.

In part one I wrote about starting with tests and using carton to manage the module dependencies. In part two we covered Perl::Critic and looking at the code from that 30,000 view.

In this post; I'll show some of the refactorings that came from the 30,000 view.

So if you look at the code, you can see lots of repetition, which we can simplify.

There are many repetitions that look like this:

sub add_one_white_penalty {
    $white_penalty++;
    return ($white_penalty);
}
sub remove_one_white_penalty {
    $white_penalty--;
    return ($white_penalty);
}

So what we can do is create a single sub that accepts some parameters and update the counters, so something like this:

sub update {
    my (%args) = @_;

    if ( $args{'mode'} eq 'inc' ) {
        $counters{ $args{'colour'} }{ $args{'statistic'} }++;
    }
    else {
        $counters{ $args{'colour'} }{ $args{'statistic'} }--;
    }
}

So this single function can replace all those repetitions (we did also need to created hash to hold the data, rather than the many scalars.

The other large bit of repetition is the binding of keys to the update function, which before refactoring looks like this:

    $cui->set_binding( sub { add_one_blue_attack(); update_screen(); }, 'f' );
    $cui->set_binding( sub { remove_one_blue_attack(); update_screen(); }, 'F' );

    $cui->set_binding( sub { add_one_blue_effattack(); update_screen(); }, 'd' );
    $cui->set_binding( sub { remove_one_blue_effattack(); update_screen(); }, 'D' );

So we can reduce the repetition like this:

    my @keys = (
        [ 'blue', 'attack',    'inc', 'f' ],
        [ 'blue', 'attack',    'dec', 'F' ],
        [ 'blue', 'effattack', 'inc', 'd' ],
        [ 'blue', 'effattack', 'dec', 'D' ],
        [ 'blue', 'koka',      'inc', 'v' ],
        [ 'blue', 'koka',      'dec', 'V' ],
        [ 'blue', 'yuko',      'inc', 'c' ],
        [ 'blue', 'yuko',      'dec', 'C' ],
        [ 'blue', 'wazari',    'inc', 'x' ],
        [ 'blue', 'wazari',    'dec', 'X' ],
        [ 'blue', 'ippon',     'inc', 'z' ],
        [ 'blue', 'ippon',     'dec', 'Z' ],
        [ 'blue', 'penalty',   'inc', 't' ],
        [ 'blue', 'penalty',   'dec', 'T' ],

        [ 'white', 'attack',    'inc', 'j' ],
        [ 'white', 'attack',    'dec', 'J' ],
        [ 'white', 'effattack', 'inc', 'k' ],
        [ 'white', 'effattack', 'dec', 'K' ],
        [ 'white', 'koka',      'inc', 'n' ],
        [ 'white', 'koka',      'dec', 'N' ],
        [ 'white', 'yuko',      'inc', 'm' ],
        [ 'white', 'yuko',      'dec', 'M' ],
        [ 'white', 'wazari',    'inc', ',' ],
        [ 'white', 'wazari',    'dec', '<' ],
        [ 'white', 'ippon',     'inc', '.' ],
        [ 'white', 'ippon',     'dec', '>' ],
        [ 'white', 'penalty',   'inc', 'u' ],
        [ 'white', 'penalty',   'dec', 'U' ],

    );

    for my $k (@keys) {
        $cui->set_binding(
            sub {
                update(
                    colour    => $k->[0],
                    statistic => $k->[1],
                    mode      => $k->[2],
                    ),
                    update_screen();
            },
            $k->[3]
        );
    }

This drops the line count from 440 down to 334 lines, smaller is better. Less code is less places for bugs to hide. The code is technically a little more complex, but simpler in that we are not repeating ourselves as much. So we can reduce the chances of introducing bugs.

When we zoom out on the code now, there code has a very different shape to it, the repetitive subs are no longer there. This helps highlight the next level of complexity, the display code... and that's what we will tackle next time.