How to fix your code by coding the fixer

Kang-min Liu // @gugod
LINE Fukuoka

TPC::Glasgow

Coding,
by coding
the coder.

My team

  • 3 frontend (js) devs, 4 server-side (perl) devs
    • I'm the newbie.
  • code review + human QA before rollout
  • 30% time on code review.

Code review pros

  • Requires understanding
    of spec and code
  • Ensure "readability"
    • increase "maintainability"

Note:
I'm not defining "readability" or "maintainability" here.
Let's just say: Being able to understand other's code
should also enables you to maintain those code.

Readability

  • Unconventional

    • prefer tech X for problem Y

  • Quirks, magic, shenanigans

    • "Why does it need to be done like this?"

Readability

  • Mismatching style

    • "This does not looks pretty"

  • Random "code waste"

    • Unused stuff

Style

  • Dress
  • "Fashion"
  • Shape & Color
  • Whitespace management

Note:
Why using "LaTeX" instead of $whatever ?
How many time you've spent on configuring the fonts & color of your Termnial / Editor?
You probably dislike Comic Sans font for a reason.

  • Perl::Tidy
  • $EDITOR

Code Wastes

  • Unused variables
  • Unused subroutines
  • Unused imports
  • Uncleaned namespace
  • Unused hash key/value

Tools to swipe code wastes

  • Perl::Critic
  • App::Critique
  • Ref::Util::Rewriter

  • eslint https://eslint.org/

  • gofix https://golang.org/cmd/fix/

    • Fix finds Go programs that use old APIs and rewrites them to use newer ones. After you update to a new Go release, fix helps make the necessary changes to your programs

  • search: code refactoring tool

PPI

$found = $doc->find(...) or return;
@wastes = grep { ... } @$found;

$_->remove for @wastes;

Deleting stuff


$elem2 = ...

$elem->insert_before( $elem2 );
$elem->insert_after( $elem2 );

Adding stuff

my $new_statement = ...;

$elem->insert_before(
    $new_statement
);
$elem->insert_before(
    PPI::Token::Whitespace->new("\n")
);

Adding a new statement.

my $doc2 = PPI::Document->new(\$code_str);
my @elems = $doc2->children;
$_->remove for @elems;

my $e = $doc->find_first(...);
$e->insert_before($_) for @elems;

Paste $code_str before $e

Note:
PPI::Document::Fragement exists but
it is not really integrated with rest of PPI

$elem->insert_before($_) for @new_stuff;
$elem->remove;

Replace one element with others

p5-nitpick

https://github.com/gugod/p5-nitpick

Finds smelly spots and remove them.

Removing unused variables

my $o = App::P5Nitpick::PCPWrap->new(
  'Perl::Critic::Policy::Variables::ProhibitUnusedVariables'
);
my @vio = $o->violates(
  undef,
  Perl::Critic::Document->new(-source => $doc)
);

for (@vio) {
  my ($msg, $explain, $el) = @$_;
  if ($el->variables == 1) {
      $el->remove;
  }
}

my $foo;         # my $foo; is removed
my ($bar, $qux); # $qux is not removed
$bar = ...
$bar->do_something;
...

Remove unused include statement.

use Try::Tiny;  # unused
use HTTP::Tiny;

my $res = HTTP::Tiny->get(...);
do_something($res);


my $has_try_block = 0;
for my $try_keyword (@{
    $doc->find(sub {
        $_[1]->isa('PPI::Token::Word') &&
        $_[1]->content eq 'try'
    }) ||[]
}) {
    my $try_block = $try_keyword->snext_sibling or next;
    next unless $try_block->isa('PPI::Structure::Block');
    $has_try_block = 1;
    last;
}
return if $has_try_block;

my @includes = @{ $doc->find('PPI::Statement::Include') ||[] };
return unless @includes;

for my $try_module (qw(Try::Tiny Try::Catch Try::Lite TryCatch Try)) {
    my @uses = grep { $_->module eq $try_module } @includes;
    if (@uses && !$has_try_block) {
        push @violations, map { $self->violation("Unused ${try_module} module", 'There are no `try` block in the code.', $_) } @uses;
    }
}

AppendUnimportStatement

use Moose;
...

no Moose;  # ←
1;

for Moose, Mouse, Moo, Moose::Role, Mouse::Role

my $doc2 = PPI::Document->new(\"no Moose;");
my $el = $doc2->find_first('PPI::Statement::Include');
$el->remove;

my $c = $doc->schild(-1); # '1;'
$c->insert_before($el);
$c->insert_before(PPI::Token::Whitespace->new("\n"));

my $doc2 = PPI::Document->new(\"use namespace::autoclean;");
my $el = $doc2->find_first('PPI::Statement::Include');
$el->remove;

my $c = $doc->schild(2);
$c->insert_before($el);
$c->insert_before(PPI::Token::Whitespace->new("\n"));

Insert use namespace::autoclean; right after package statement.

RewriteWithAssignmentOperators

# From
$a = $a + 3;

#To
$a += 3;

> ppi-dump -e '$a = $a + 3;' | grep -v Whitespace

PPI::Document
  PPI::Statement
    PPI::Token::Symbol  	'$a'
    PPI::Token::Operator  	'='
    PPI::Token::Symbol  	'$a'
    PPI::Token::Operator  	'+'
    PPI::Token::Number  	'3'
    PPI::Token::Structure  	';'

my @found = ... grep {
    $_->schildren == 6
} @{ $document->find('PPI::Statement') ||[] };

$a = $a + 3 ;
1  2 3  4 5 6

my @found= ... grep {
    my $c = $_->schild(1);
    $c->isa('PPI::Token::Operator') &&
    $c->content eq '='
} ... 
@{ $document->find('PPI::Statement') ||[] };

$a = $a + 3 ;
my @found= ... grep {
    my $c = $_->schild(3);
    $c->isa('PPI::Token::Operator') &&
    $c->content !~ m{\A( -> | > | < )\z}x;
} ... 
@{ $document->find('PPI::Statement') ||[] };


$a = $a + 3 ;

$a = $a > 3;
$a = $a < 3;
$a = $a ->foobar;

my @found= ... grep {
    my $c1 = $_->schild(0);
    my $c2 = $_->schild(2);
    $c1->isa('PPI::Token::Symbol') && $c1->raw_type eq '$' &&
    $c2->isa('PPI::Token::Symbol') && $c2->raw_type eq '$' &&
    $c1->content && $c2->content
} ... 
@{ $document->find('PPI::Statement') ||[] };

$a = $a + 3 ;
for (@found) {
    my @child = $_->schildren;

    # $a = $a + 3 ;
    #  0 1  2 3 4 5

    # '=' , '+' => '+='
    my $op = PPI::Token::Operator->new($child[3]->content . $child[1]->content);

    $child[3]->remove; # '+'
    $child[2]->remove; # '$a'
    
    # '=' => '+='
    $child[1]->insert_after($op);
    $child[1]->remove;
}

Perl::Critic::TooMuchCode

  • ProhibitUnusedConstant
  • ProhibitUnusedImport
  • ProhibitUnusedInclude
  • ProhibitUnnecessaryUTF8Pragma
  • ProhibitLargeBlock
  • ProhibitLargeTryBlock

https://metacpan.org/pod/Perl::Critic::TooMuchCode

More code-fixable patterns

  • $n = scalar @arr;
  • do_this();; Extra semi-colons / empty statements.
  • @a = @{[ "foo", "bar", foo() ]}
  • Unused hash key/value
  • typo in Document
  • typo in variable name (maybe)

Wrap-up

  • Altering code with PPI is generally easy.

  • Finding what can be altered can be tedious

    • Check Perl::Critic::Utils

  • Identifying bad tastes is difficult but seems to be worthwhile

References

"Thank you for listening";

Select a repo