リファクタリング Good・Badケース

6545 ワード
リファクタリング プログラミング チーム開発 プログラミング テキストリンク

リファクタリングする際の自分なりのGood・Badケース
※リファクタリングといいつつ一部「修正　+ リファクタリング」のケースがあったりします

※サンプルコードはphp7系で記載してます

【Good-1】 適切に関数化を実施する

サンプルコード
public function test() {
    // A ...

    // B ...

    // C ...
}
リファクタリング後
private function A() {
    // A ...
}

private function B() {
    // B ...
}

private function C() {
    // C ...
}

public function test() {
   $this->A();
   $this->B();
   $this->C();
}

処理が長い場合には
コードの初期に記載されている変数が後の処理で急にでてきて「これなんだ…」ってなったりで、
処理全体を一括で把握していく必要があるのでかなりSAN値がガリガリけずられます

また、適切に関数化されていると、
後で項目として説明しますが影響範囲をおさえながら対応できます

「ある処理のまとまりをみつけて名前つけていく」 感じで対応していくといいかと思います

プロダクト理解が浅い内は難しいかもしれませんが、
きちんとコードを読んで関数化（名前付け）をしていきましょう

【Bad-1】 影響範囲を考えずに修正してしまう

function B()をfunciton B'()に変更する必要がある

サンプルコード
private function A() {
    // A ...
}

private function B() {
    // B ...
}

private function C() {
    // C ...
}

public function test() {
   $this->A();
   $this->B();
   $this->C();
}
リファクタリング後
private function A() {
    // A ...
}

private function B'() {
    // B' ...
}

private function C() {
    // C ...
}

public function test() {
   $this->A();
   $this->B'();
   $this->C();
}

一見よさそうにみえますが、function testが複数箇所で利用されていて、
function Bの処理結果をその後に利用されていたりすると全件テストになってしまいます

【Good-2】 影響範囲を考慮した対応

リファクタリング後
private function A() {
    // A ...
}

private function B () {
    // B ...
}

private function B'() {
    // B' ...
}

private function C() {
    // C ...
}

// ToDo: 使用箇所の修正時にはtest'に置き換えること
public function test() {
   $this->A();
   $this->B();
   $this->C();
}

public function test'() {
   $this->A();
   $this->B'();
   $this->C();
}

先ほどとはうって変わって、function test'を作成しています

【Good-1】の対応のように適切に関数化されている場合には、
その部分については共通処理としておけるので、
仮にfunciton A()などでバグがあった場合にも修正は、
funciton test・｀function test'`にも適用されます

ただ、一時的とはいえfunction testとfunction test'が混在してしまうので、
早いタイミングでfunction testを抹殺しましょう

影響範囲がそこまででもない場合でも、
function test'を記載して部分的におきかえていき、
全て置き換えが完了したらfunciton testを消す対応がよいかと思います

影響範囲の読み間違えで、「あれこの対応結構やばい…」ってならずに済みます！

【Good-3】 配列をクラスに置き換える

サンプルコード
$tests = [];

$tests['aaa'] = $aaa;
$tests['bbb'] = $bbb;
$tests['ccc'] = $ccc;
リファクタリング後
class tests
{
    private string $aaa;
    private string $bbb;
    private string $ccc;

    public function setAAA(string $aaa): void
    {
        $this->aaa = $aaa;
    }
    
    public function setBBB(string $bbb): void
    {
        $this->bbb = $bbb;
    }

    public function setCCC(string $ccc): void
    {
        $this->ccc = $ccc;
    }
   
    public function getAAA(): string
    {
        return $this->aaa;
    }

    public function getBBB(): string
    {
        return $this->bbb;
    }

    public function getCCC(): string
    {
        return $this->ccc;
    }
}

「あれ、コード量増えてないですか…」ってなるかもしれないですが、

配列は悪です！！

1箇所で情報まとめるだけに使用しているとかならいいですが、
配列を生成して、他の処理に引き渡してそこで要素増えてreturnするとか…

配列は内部構造がわからないので「このデータもってるっけ？」とか、
「この添字のデータってどうやって作られているっけ？」とかカオスです
※型指定もできないですし

classの場合には、あとからプロダクトに参画した人が把握しやすいです

スコープもきちんと設定されていれば不正値なども考慮しなくていいのも良いです

配列は悪です！!

【Good-4】 変数の役割を明確に記載する

サンプルコード
$a = 1;
$b = 200;
$c = 1;
$d = 3;

echo($a + ($c * $d));

$e = ($a + $c) * $b

echo ($e);

これは以下を表しているとします

コード内容
りんご1個とみかん3個セットは単価200円で販売されている
りんご1個とみかん3個セットを1つ購入した時の、
購入する果物の個数と金額を出力する

上記のサンプルコードだと、文章読まないとよくわからないですね

リファクタリング後
$apple_num = 1;
$orange_set_num = 1:
$orange_set_quantity = 3;

$total_quantity = $apple_num + ($orange_set_num * $orange_set_quantity);
echo ($total_quantity);

$unit_price_of_apple = 200;
$unit_price_of_orange_set = 200;

$price = ($apple_num * $unit_price_of_apple) + ($orange_set_num * $unit_price_of_orange_set);
echo ($price);

なぜか世の中的に、関数名や変数名が短いことが正義みたいになっていますが、
リファクタリング後の方がコードの処理内容をイメージしながら読めたりしませんか？

また、上記では$bで格納されていた200を、
$unit_price_of_appleと$unit_price_of_orange_setに分割しています
数値としては同じ200なのですが、
りんごの単価のみ変わる可能性もあったりするのでこういった分け方をしています

変数名のつけ方については、
個人的には表現したいことをGoole翻訳してでてきた単語をならべています

また 【Good-1】で説明したとおり、
購入する果物の数の取得関数や、金額を算出する関数を作成してあげるとより、
可読性・保守性が高まると思います

さいごに

Badパターンをもっと書こうとおもったのですが、
Goodパターンが多めになってしまいました

「これどうなの…」みたいなところありましたら、コメントいただければと思います