過剰なリファクタリングライン コピペ(共通化・抽象化)編

書き換えたコードが本当に書き換える前より優れているかどうか非常に不安なので、吐き出しつつ考えてみます。

元コード

元コード

<?php


function init()
{
    if (is_file('a')) {
        unlink('a');
    }
    if (is_file('b')) {
        unlink('b');
    }
    if (is_file('c')) {
        unlink('c');
    }
    // …いっぱい続く
}

何でもいいですが、対象引数を変更したコピペコードがn個あるとします。ifでなくてもいいです。

こういったコードは目が下町ボブスレーよりよく滑ります。

メソッドを短くしたい欲求でウズウズしませんか？しませんか…。

とにかく直してみます。

無名関数を使った書き換え

is_fileとunlinkは不可分な関係に見えるので、一行にまとめてみます。

無名関数を使った変更(ダメな変数名)

function init()
{
    $isFileUnlink = function ($filename) {
        if (is_file($filename)) {
            unlink($filename);
        }
    };

    $isFileUnlink('a');
    $isFileUnlink('b');
    $isFileUnlink('c');
}

奇しくも行数が一緒ですが…'d'以降から行数短縮効果が見えてきます。
無名関数にしているのは、一番迷惑がかかりにくい、メソッド単位の領分でできるリファクタリングなんじゃないかなと思うからです。ｺﾂｺﾂ

わかりきったifと処理をコード頭で理解すれば、後は何が渡されるのかを確認するだけになりました。

ただ、同じ関数名がずらずら並ぶのは、if ~ unlink と同じ悪効果が出そうです。
行数の短縮には成功しているのですが、これはグラデーションな良いのどのあたりでしょうか。¹

ループを使った書き換え

引数を変えていく方法には別のやりかたもありそうです。
処理をまとめるのではなく、引数をまとめてみます。

ループを使った変更

function init()
{
    $filenames = [
        'a',
        'b',
        'c'
    ];
    foreach ($filenames as $filename) {
        if (is_file($filename)) {
            unlink($filename);
        }
    }
}

ネストは深くなりましたが、「同じ文字が何度も書かれる」状態からは脱せたと思います。
削除するファイルが増えた場合も、関数名が増えず、ファイル名を配列に追加するだけで済みました。

ただ、処理と対象が離れてしまったとも言えるかもしれません。
最初にファイル名の集合を見せられても、頭のメモリを喰うだけかもしれません。
引数が一つではなく複数になり、連想配列を持つようになっても理解のしやすさは損なわれないでしょうか？
(反論の反論になりますが、データと処理の分離はPHPUnitを使っていればデータプロバイダとして違和感がないかもしれません)

複雑度が増した

function init()
{
    $args = [
        [
            'filename' => 'a',
            'msg'      => '消したよ',
            'code'     => '0001',
        ],
        [
            'filename' => 'b',
            'msg'      => '削除しました',
            'code'     => '0002',
        ],
        [
            'filename' => 'c',
            'msg'      => '消去完了',
            'code'     => '0003',
        ],
    ];
    foreach ($args as $arg) {
        if (is_file($arg['filename'])) {
            unlink($arg['filename']);
            echo $arg['msg'];
            Logger::log($arg['code']);
        }
    }
}

…簡単な例で持ち出すのは趣旨から逸れそうです。
配列を作る関数を作ったりクラス作ったりとデザインパターンまで足を突っ込みそうなので。
ただ、3つの時点で素直に書くと苦しそう。
$arg配列宣言を関数化すると、短くなったと言っても関数の記述が最初の例のように複数個出てきてループにしたメリットが消えて行く気がします。²

面倒な連想配列のキー記述を減らしたい変更

function init()
{
    $makeArg = function ($filename, $msg, $code) {
        return [
            'filename' => $filename,
            'msg'      => $msg,
            'code'     => $code,
        ];
    };
    $args = [
        $makeArg('a', '消したよ', '0001'),
        $makeArg('b', '削除しました', '0002'),
        $makeArg('c', '消去完了', '0003'),
    ];
    foreach ($args as $arg) {
        if (is_file($arg['filename'])) {
            unlink($arg['filename']);
            echo $arg['msg'];
            Logger::log($arg['code']);
        }
    }
}

$makeArgが連続している…ループに書き換えて$args[] =　$makeArgにするために引数の配列を作ろう……
あれ？無限ループ…？

両方を使った書き換え

混乱してきたので立ち戻りましょう。
最後に、最初に思いついた2つをあわせてみた例はどうでしょうか。

合作

function init()
{
    $isFileUnlink = function ($filename) {
        if (is_file($filename)) {
            unlink($filename);
        }
    };
    $filenames = [
        'a',
        'b',
        'c',
    ];
    foreach ($filenames as $filename) {
        $isFileUnlink($filename);
    }
}

うーん、やっぱり複雑でしょうか。
本来

if (is_file($filename)) {
    unlink($filename);
}

をしたかっただけのコードに対して、やっていることが大仰ではないでしょうか。
書いた自分ですら不安なら、初めて読む第三者は…？

おわり

リーダブルコードでも「やりすぎ」を諌めています。
ではどれぐらいが…？

もちろん良い環境ではコードレビューがあるので、なんらかのフィードバックが得られるでしょう。
本当に気になるなら質問サイトでもよろしい。

そのようなことがなく、せず、
自分との戦い、未だ見えざる他人の目を考えての孤独な自問自答のすえ、書き換える前のコピペが最良に見えてくることも³ままあるのです。

1. そもそも良いどころか悪いかもしれない ↩

2. 律儀に連想配列にしているのが悪い ↩

3. リファクタリングを書ききったコミット直前に ↩