LoginSignup
2
1
@yama-github

コード整理の流れを実演してみます(PHP)

Last updated at Posted at 2020-06-11

PHPって「とにかく動けばいい」って感じの雑なコードが多い気がします。いつか困る日が来ます。

下記のような、少し見通しの悪いコードがあるとします。

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        $parent = 'root';
    }

    if ($parent !== 'root') {
        $isAllowed = $modx->manager->isAllowed($parent);
        if (!$isAllowed) {
            $parent = 0;
        }
    }

    if ($parent === 'root') {
        $a = 'a=2';
    } elseif ($parent == 0) {
        $a = 'a=120';
    } else {
        $a = "a=120&id={$parent}";
    }

    return 'index.php?' . $a;
}

これは実際にMODXで使っているコードですが、こういうコードが多いと、システム全体での処理の流れが見えにくくなります。原因不明の不具合が増えたり、負荷が大きくなったりもします。

これをどうにか、MODXを知らないエンジニアでもなんとなく分かるコードにしたい。まず末尾のほうから見ていきます。このコードはどういう値を返そうとしているのかを把握するためです。

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        $parent = 'root';
    }

    if ($parent !== 'root') {
        $isAllowed = $modx->manager->isAllowed($parent);
        if (!$isAllowed) {
            $parent = 0;
        }
    }

    if ($parent === 'root') {
        return 'index.php?' . 'a=2';
    } elseif ($parent == 0) {
        return 'index.php?' . 'a=120';
    } else {
        return 'index.php?' . "a=120&id={$parent}";
    }
}

上記のように書き換えて、$a という変数が不要になりました。「とりあえず適当に変数を作って入れる」ということをやめることで、処理の流れが見やすくなります。
※Xdebug使う場合はこまめに変数にまとめたほうがいいかも

return が３つ続いていますが、ここは整理できます。

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        $parent = 'root';
    }

    if ($parent !== 'root') {
        $isAllowed = $modx->manager->isAllowed($parent);
        if (!$isAllowed) {
            $parent = 0;
        }
    }

    if ($parent === 'root') {
        return 'index.php?' . 'a=2';
    }
    if ($parent == 0) {
        return 'index.php?' . 'a=120';
    }
    return 'index.php?' . "a=120&id={$parent}";
}

returnを整理しました。elseifやelseが不要になることが分かりますでしょうか？

次。
'index.php?' . 'a=2' や 'index.php?' . "a=120&id={$parent}" をまとめます。

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        $parent = 'root';
    }

    if ($parent !== 'root') {
        $isAllowed = $modx->manager->isAllowed($parent);
        if (!$isAllowed) {
            $parent = 0;
        }
    }

    if ($parent === 'root') {
        return 'index.php?a=2';
    }
    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}

上記のように書き換えました。ここまでは簡単です。

この関数は $parent の値によって３通りのリターンを返すことが分かりますでしょうか？ここに注目して、さらに整理を進めます。

    if ($parent !== 'root') {
        $isAllowed = $modx->manager->isAllowed($parent);
        if (!$isAllowed) {
            $parent = 0;
        }
    }

上記の部分を見ると、 $parent の値が root であっても、場合によっては変わることが分かると思います。条件を満たさない場合、 $parent の値は root ではなく 0 に変わります。

そもそも root という文字列自体には何か意味があるのかな？

$parent の値が 0 の場合は、

return 'index.php?a=120';

上記のように返すことになります。ということは、ここでよく考えます。

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        $parent = 'root';
    }

    if ($parent !== 'root' && !$modx->manager->isAllowed($parent)) {
        return 'index.php?a=120';
    }

    if ($parent === 'root') {
        return 'index.php?a=2';
    }
    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}

少し難しいかもしれませんが、上記のように整理できます。$parent に 0 を代入するのをやめて、そこで return で返すようにしました。

返す値が分かりやすくなったので、今度は分岐条件のほうに目を向けていきます。 $parent の値ですが、これは関数に引数として与えられるものではありません。引数は $current という配列です。

$parent は $current['parent'] の値だけを見ます。$current['parent'] はMODX内では親ページのIDを示すもので、0を含む「数値」が入ります。

ということは・・・

$current['parent'] が存在する場合は、それをそのまま $parent に代入し、存在しない場合は root という「文字列」を代入する。ということが読み取れるでしょうか？

$parent が root の場合は、

return 'index.php?a=2';

上記を返しますよね？ということは、

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        return 'index.php?a=2';
    }

    if ($parent !== 'root' && !$modx->manager->isAllowed($parent)) {
        return 'index.php?a=120';
    }

    if ($parent === 'root') {
        return 'index.php?a=2';
    }
    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}

上記のようになります。root という文字列は、このコード内だけで必要なただの目印でした。 $parent に root を代入する処理がなくなり、すぐに return できるようになったので、つまり、

function getReturnAction($current) {
    global $modx;

    if (isset($current['parent'])) {
        $parent = $current['parent'];
    } else {
        return 'index.php?a=2';
    }

    if (!$modx->manager->isAllowed($parent)) {
        return 'index.php?a=120';
    }

    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}

上記のように書き換えることができます。だいぶシンプルになりました。

冒頭の if-else に注目します。else節で return していますが、この判定を逆にするとさらに短くできることが分かりますでしょうか？ 実際に書き換えてみます。

function getReturnAction($current) {
    global $modx;

    if (!isset($current['parent'])) {
        return 'index.php?a=2';
    } else {
        $parent = $current['parent'];
    }

    if (!$modx->manager->isAllowed($parent)) {
        return 'index.php?a=120';
    }

    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}

上記のように書き換えます。ifとelseを逆にしました。すると、

function getReturnAction($current) {
    global $modx;

    if (!isset($current['parent'])) {
        return 'index.php?a=2';
    }

    $parent = $current['parent'];

    if (!$modx->manager->isAllowed($parent)) {
        return 'index.php?a=120';
    }

    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}

上記のように、else 側の判定を省略できます。

まだ整理できます。さっき、root という値を意識せずにすむようになりましたが、そうすると $parent は単純に $current['parent'] の値を代入するだけです。意味のない代入になるので、そのまま使いましょう。

function getReturnAction($current) {
    global $modx;

    if (!isset($current['parent'])) {
        return 'index.php?a=2';
    }

    if (!$modx->manager->isAllowed($current['parent'])) {
        return 'index.php?a=120';
    }

    if ($current['parent'] == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $current['parent'];
}

上記のように整理できました。

最初の３つのreturnは $current['parent'] の値が無効な処理になっていて、$current['parent'] の値をそのまま使う処理は最後に配置しています。falseな処理を前半で順番にひとつずつ落としていって、trueな処理をできるだけシンプルな形でひとつだけ残し、最後に返す。そういう書き方になっています。

返す値として return 'index.php?a=120' が重複しているのが気になりますので、さらにまとめます。

function getReturnAction($current) {
    global $modx;

    if (!isset($current['parent'])) {
        return 'index.php?a=2';
    }

    if ($current['parent'] == 0 || !$modx->manager->isAllowed($current['parent'])) {
        return 'index.php?a=120';
    }

    return 'index.php?a=120&id=' . $current['parent'];
}

上記のようになります。
まとめる順番として $current['parent'] == 0 を先にしている点がポイントです。
もし仮に $modx->manager->isAllowed() の処理が非常に重いものだとします。
そうすると、先に $current['parent'] == 0 を持ってくることで、この判定が正である場合は isAllowed() の判定を回避することができ、負荷を軽減できます。

次。
少し脱線になりますが、global宣言はかっこ悪いので、最近のMODXでは下記のようにしています。


function evo() {
    global $modx;
    return $modx;
}

function manager() {
    return evo()->manager;
}

結局global宣言は使うのですが、大きなシステムの中で、ここだけに限定することができます。これを使って、

function getReturnAction($current) {
    if (!isset($current['parent'])) {
        return '?a=2';
    }
    if ($current['parent']==0 || !manager()->isAllowed($current['parent'])) {
        return '?a=120';
    }
    return '?a=120&id=' . $current['parent'];
}

最終的にはここまで整理することができます。変数代入を一度も使わずにすみました。

あと、呼び出し元も軽い修正を加えて、$current['parent']を渡す場合は必ずゼロから始まる整数になるようにしました。
!isset($current['parent'] と !$current['parent'] の混在を見ると整理ができてないように見えてしまうためです。

今回みたいに説明を添えると長い話になりますが、実際は頭を使わずどんどん手を動かして、これくらいなら1～2分で書き換えられます。中学2年レベルの方程式を解いていく感覚です。

追記 2020-06-12

https://qiita.com/yamamotomasa/items/0bba421f958475ff72b2#comment-ead4717086e37bd9e018
@vf8974 さんからコメントいただきました。さらにまとめることができます。

function getReturnAction($current) {
    return sprintf('index.php?a=%d%s',
        isset($current['parent'])
            ? 120
            : 2,
        !empty($current['parent']) && manager()->isAllowed($current['parent'])
            ? '&id=' . $current['parent']
            : ''
    );
}

実質ワンラインですね。

(2023-02-14追記)ChatGPTに聞いてみた

下記、ChatGPTの回答です。

この関数にはいくつかの問題があります。

  1. global $modxを使用しているため、関数は外部の状態に依存しています。このため、コードの複雑さや保守性が低下する可能性があります。
  2. if (isset(current['parent']))は、current配列内のparentキーが存在する場合にのみ$parent変数に代入されますが、parentキーが存在しない場合、$parent変数は'root'に設定されます。しかし、配列にparentキーが存在しない場合、関数の呼び出し元が正しい値を提供しているかどうかわからず、不正な値を使用する可能性があります。
  3. $modx->manager->isAllowed()は、現在のユーザーが指定されたアクセス権限を持っているかどうかを判断するために使用されます。しかし、この関数がどのように機能するかについては、与えられたコンテキストに依存するため、正しい動作が保証されていません。
  4. $parent === rootの場合、$a変数にはa=2が設定されますが、この文字列がどのような意味を持つのかは不明です。
  5. $parent == 0の場合、$a変数には a=120 が設定されますが、この文字列がどのような意味を持つのかは不明です。
  6. else節で$parentがrootでも0でもない場合、 a変数には a=120&id={$parent}が設定されます。この場合、$parentは数値であることが期待されますが、それが保証されていません。

以上の問題点を解決するためには、より明確な関数の動作を定義し、外部の状態に依存することなく、関数の引数を使用することが望ましいです。また、関数内で値を設定する前に、与えられた引数が期待される形式であるかどうかを検証することも重要です。

改善例は下記のとおり。

function getReturnAction($parent, $manager) {
    if ($parent === 'root') {
        $id = 2;
    } else {
        $id = (int) $parent;
        if (!$manager->isAllowed($id)) {
            $id = 120;
        }
    }
    return 'index.php?a=120&id=' . $id;
}

う～ん、、、$idが決め打ちで2になったりはしないよ～。引数の仕様が変わっちゃうのも困るし。
でもアドバイスの内容は参考になります。AIはまだまだ進化し続けるはずなので、いい感じにお付き合いしていきたいです。

2
1
2

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
2
1