This is part two of an exploration of some ideas around how I approach picking up a legacy Perl code base.

In part one I wrote about starting with tests and using carton to manage the module dependencies.

In this post; we will take a look at the code base itself and some ideas on how to quickly identity areas to improve.

Perl::Critic

Perl is a mature language, with mature tooling. One of the "hidden gems" of Perl is Perl::Critic, which is a static analysis tool that gives great insights into the code. One really powerful aspect is that we have a book to reference; "Perl Best Practices" and Perl::Critic can help us follow this guidance and will reference exact parts of the book. It's extensible and there are plenty of opinionated Perl developers who have shared their policies.

So lets run it and see what happens, after first adding Perl::Critic to the cpanfile.

$ carton exec perlcritic -5 Notator.pl
Notator.pl source OK

So at the least strict level, the file is OK. This is a good sign; nothing terrible wrong. So lets try another level of strictness:

$ carton exec perlcritic -4 Notator.pl
Magic variable "$OUTPUT_AUTOFLUSH" should be assigned as "local" at line 12, column 19.  See pages 81,82 of PBP.  (Severity: 4)
Subroutine "new" called using indirect syntax at line 87, column 12.  See page 349 of PBP.  (Severity: 4)
Subroutine "new" called using indirect syntax at line 355, column 20.  See page 349 of PBP.  (Severity: 4)

This time we get three lines, you can see the references to pages in the Perl Best Practices (PBP) pages that relate. There are also references to the lines in the file itself. So the last two lines are the same and are pretty easy fixes; so lets go fix those.

    $cui = new Curses::UI( -color_support => 1 );

We can rewrite this as:

    $cui = Curses::UI->new( -color_support => 1 );

That warning about the Magic variable is pretty self explanatory, so we can pop local at the start of the line as suggested.

If we re-run perlcritic we are now all clear again.

But if we look at the $cui = new Curses::UI( -color_support => 1 ); line, you'll notice that there is no my; which if we look about the file a bit gives us a clue... this file is all global variables which is generally a bad thing. Looking further the variables are at least tidy and sensible, but there are a lot of scalar variables which is not ideal. A to be fair to the 2007 author (which yes was me), they did leave a large comment to make it clear the variables are global.

# -----------------------------------------------
# Global Variables
# -----------------------------------------------

my $run_flag = 1;

my $segments = 0;
my $active   = 0;
my $events   = 0;

my $blue_attack    = 0;
my $blue_effattack = 0;
my $blue_koka      = 0;
my $blue_yuko      = 0;
my $blue_wazari    = 0;
my $blue_ippon     = 0;
my $blue_penalty   = 0;

my $white_attack    = 0;
my $white_effattack = 0;
my $white_koka      = 0;
my $white_yuko      = 0;
my $white_wazari    = 0;
my $white_ippon     = 0;
my $white_penalty   = 0;

my $cui;
my $win1;
my $win2;
my $win3;
my $info_blue;
my $info_white;
my $textviewer;

Readonly my $BASEYEAR => 1900;

We can look at perhaps creating a hash so we decrease the variables... but that is minor. We could look at scoping the variables and so forth too. That use of Readonly we could alter too, does it really need to be readonly? Could we use constant instead?

But first lets look more broadly.

Getting the big picture

A tip for looking at a new code base is to literally scroll out and look at the shape of the code. This is an old hackers trick, but somewhere I have read research where teaching developers about code smells by looking at files at this zoom level was proven to be effective (as effective) as teaching the underlying principles.

So lets take a look:

And...

What is leaping out to me lots of repetition, and the more code we write, the more bugs we introduce. So that looks like a really good place to invest some time and energy. So lets see what we can do there.

Looking at the second pic those look like a series of subroutines that have near identical structures, here are a few of them:

sub remove_one_white_effattack {
    $white_effattack--;

    return ($white_effattack);

}

sub add_one_white_koka {
    $white_koka++;

    return ($white_koka);

}

sub remove_one_white_koka {
    $white_koka--;

    return ($white_koka);

}

So yes they are all really similar; basically just altering those global variables; and looking at the other block of repetition we see:

    $cui->set_binding( sub { add_one_white_effattack(); update_screen(); },
        'k' );
    $cui->set_binding( sub { remove_one_white_effattack(); update_screen(); },
        'K' );

    $cui->set_binding( sub { add_one_white_koka(); update_screen(); }, 'n' );
    $cui->set_binding( sub { remove_one_white_koka(); update_screen(); },
        'N' );

So this gives us some ideas on where we can focus our attentions, and that shall be the topic of the next post in this series.