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

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

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

```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を知らないエンジニアでもなんとなく分かるコードにしたい。まず末尾のほうから見ていきます。このコードはどういう値を返そうとしているのかを把握するためです。

```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') {
        return 'index.php?' . 'a=2';
    } elseif ($parent == 0) {
        return 'index.php?' . 'a=120';
    } else {
        return 'index.php?' . "a=120&id={$parent}";
    }
}
```

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

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

```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') {
        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}"` をまとめます。

```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') {
        return 'index.php?a=2';
    }
    if ($parent == 0) {
        return 'index.php?a=120';
    }
    return 'index.php?a=120&id=' . $parent;
}
```

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

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

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

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

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

`$parent` の値が `0` の場合は、

```php
return 'index.php?a=120';
```

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

```php
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` の場合は、

```php
return 'index.php?a=2';
```

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

```php
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 できるようになったので、つまり、

```php
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 していますが、この判定を逆にするとさらに短くできることが分かりますでしょうか？ 実際に書き換えてみます。

```php
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を逆にしました。すると、

```php
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']` の値を代入するだけです。意味のない代入になるので、そのまま使いましょう。

```php
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'` が重複しているのが気になりますので、さらにまとめます。

```php
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では下記のようにしています。

```php

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

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

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

```php
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 さんからコメントいただきました。さらにまとめることができます。

```php
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`は数値であることが期待されますが、それが保証されていません。

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

---

改善例は下記のとおり。

```php
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はまだまだ進化し続けるはずなので、いい感じにお付き合いしていきたいです。