古いコードのリファクタリング - tsucchi’s diary(元はてなダイアリー)

ずいぶん期間が空いてしまった。。。

古いコードをテストコードを書きつつ、モダンな感じに書き換えていくというこの企画。お題は前回同様 my_prog.pl を使います。

リファクタリングでやることは簡単、というか大体決まっています。

use strict/warnings 化
メソッドの抽出
モジュールの使用
アルゴリズムの取り換え
その他

1 use strict/warnings 化

まず、shebang の perl -w の「-w」をやめて、use warnings します。(Perl 5.6以上が必要)。理由は404 Blog Not Found:perl - use warnings; # -w でなくてでも読んでください。

これは多分修正無しで通るはず。

次、 strict 化。
いきなりファイルの先頭で use strict を書くと、大量の警告が出てあせります。ほとんどは宣言されていない変数が原因のはずです。しかし、これを一気に宣言してしまうと思ったように動作しなかったりします。多分、グローバル変数として使われているものと、ローカル変数として使われているものがあるからです。

コードが長いと、これを追っていくのは苦痛です。

use strict はブロック単位で切替えができるので、関数単位とか、ある程度の処理で区切って、use strict していくのがいいんじゃないかな、と思います。

たとえばこんな感じ。

sub read_args {
  use strict;
  #...

my_prog.pm の read_args の先頭で use strict すると、

% prove 01_my_prog.t
01_my_prog....Variable "$a_str" is not imported at my_prog.pm line 16.
Variable "$a_str" is not imported at my_prog.pm line 17.
Global symbol "$a_str" requires explicit package name at my_prog.pm line 16.
Global symbol "$a_str" requires explicit package name at my_prog.pm line 17.
Compilation failed in require at 01_my_prog.t line 11.
BEGIN failed--compilation aborted at 01_my_prog.t line 11.
01_my_prog.... Dubious, test returned 255 (wstat 65280, 0xff00)
 No subtests run 
...

となりました。$a_str が宣言されてないのがダメっぽいです。オプションの変数なので、コードの後でも使われているので、これはグローバル変数です。our 宣言します。

our $a_str = undef;

まあ本当はグローバルな時点でダメダメなのだけど、その対応をここでやるのは大変なので、後回しにしたほうがいいです。

% prove 01_my_prog.t 
01_my_prog....ok   
All tests successful.
Files=1, Tests=6,  0 wallclock secs ( 0.01 usr  0.04 sys +  0.00 cusr  0.09 csys =  0.13 CPU)
Result: PASS

テストも通ります。

基本的にはこの手順を繰り返して、use strict でエラーになる変数をローカル変数とグローバル変数に振り分けていきます。

2. メソッドの抽出

メソッドを抽出したり、関数のシグニチャとか戻り値を調整したり、していけばかなり綺麗になると思います。メソッドが超長いのに複雑で、抽出できそうにない場合は、「メソッドオブジェクト」を使えばいいかな。(Perl なので、そこまで神経質にならなくても大丈夫そうだけど)

3. モジュールの使用

次に、モジュールを使えばロジックが簡単になりそうな部分を置き換えていきます。このコードでは、オプション解析を Getopt に置き換えました。(前書いたからここでは省略)。

4. アルゴリズムの取り換え

モジュールで置き換えができないけど、もうちょっときれいに書けそうな部分では「アルゴリズムの取り換え」を行います。リファクタリングなので、ワンステップずつ丁寧に。書き換えたらテストを忘れずに。

5. 他にやること

グローバル変数はケースバイケースですが、オブジェクトモジュールにしてインスタンス変数にしちゃうのが一番普通だと思います。

オブジェクトモジュールに変更できると、テストコードと本番コードで設定を切替えたりするのが楽になります。

繰り返しになりますが、リファクタリングなので、ワンステップずつ丁寧に。書き換えたらテストを忘れずに。自分のコードではない場合なら、なおさらです。

まとめ(？)

自分以外の人が作った古いコードを書き換えるのは大変です。が、今回やってみて、「テストがあれば、なんとかなるかもなー」と思うようになりました。

Perl は @ARGV も自由に設定できるので、オプション解析みたいな他の言語でテストしにくい部分も結構ちゃんとテストできたりします。

頑張ってテストを書いて、strict 化して、モダンなコードを書いていきましょう！