「ネストの深さは闇の深さ」のコードを決定表でリファクタすると

2021.05.01

引用元の記事について

ぼくが、ある開発チームのWikiで、決定表について説明したんですね。説明のなかで「決定表を作ったときは、ネストが深くなっても気にせず、アーリーリターンせず、決定表どおりに実装します」と書いたんです。

別のメンバーが「ネストが深くなっても気にせず、アーリーリターンせず」が気になったのでしょう、次の記事を紹介してくれました。

ネストの深さは闇の深さ - Qiita
はじめにこんにちは、先日投稿したソースコードの混沌はどこから這い寄るのかという記事が思いのほかストックされて浮かれ気味のMic-Uです。そのときは「こうしようぜ！」と訴えるだけでしたので、今回は…
qiita.com

深いネストを解消していく記事です。

その投稿者は、仕事の現場で「元のコード」に遭遇し、がく然とします。そして戦いを挑み、深いネストを解消しながら「改善後のコード」にリファクタしていく過程を記事にしています。

引用記事の「元のコード」や「改善後のコード」を批判するつもりはありませんので、ご理解ください。多くのプログラマは、限られた時間の中でまじめにコードに取り組んでいると思います。

このようにリファクタしたよ、というのは後出しジャンケンです。なにより、ぼくのリファクタがが正しいとは限りません。ぼくは、アンチ早期リターンですが、どうも少数派のようですし。

元のコード

元のコードです。このコードを勝手にリファクタしていきます。

function get_tbl_data_mapping_info($db, $table_id, $disp_mode=0, $org_no=null){
  //レコード取得
  $result = select_tbl_data_mapping_info($db, $table_id, $org_no);

  if ($disp_mode != 0){
  //表示調整(○○○○CD の場合に、前の文字数が長いために「CD」が表示されなくなるのを対処
    for ($i=0;$i<count($result);$i++){
      $pos = strpos($result[$i]['name'],"CLASS_CD_");
      if ($pos === false){
      } else {
      //区分CDの場合('CD'が最後に現れる箇所)
        // $result[$i]['disp_name']はシステム側で自動生成 末尾が「CD」の文字列か空文字列
        $pos = strrpos ($result[$i]['disp_name'],"CD");
        if ($pos === false){
        } else {
          //文字列CDが有る場合
          $f_name = substr($result[$i]['disp_name'], 0, $pos);
          if (mb_strlen($f_name) > 4){
          //4文字以上の場合は5文字目以後を切り落とし
            $f_name = mb_substr($f_name, 0, 4) . "..CD";
          } else {
            if ($f_name != ''){
              $f_name .= 'CD';
            } else {
              $f_name .= '　';
            }
          }
          //上書き
          $result[$i]['disp_name'] = $f_name;
        }
      }
    }
  }
  return $result;
}Code language: plaintext (plaintext)

読みやすいとは言えないけど、戦いを挑むというほどひどくはないと思うけど．．．

記事を読んでコードを理解しているからよ

それに、40行もないしね。

あと、本当にひどいコードを記事にのせても、読むのがつらいでしょ。

ver1

for文ごと別メソッドに分ける

$disp_mode が 0かどうかは、ガード節（nullチェックや範囲外チェック）ではないので、アーリーリターンしないほうがいいと思います。

本体の長いfor文は、for文ごと別メソッドにします。

また、$resultは添字配列なので、$result_arrにリネームしたいところです。が、本題ではないので、$resultのままにします。

function get_tbl_data_mapping_info($db, $table_id, $disp_mode=0, $org_no=null){
  //レコード取得
  $result = select_tbl_data_mapping_info($db, $table_id, $org_no);

  if ($disp_mode != 0){
    $result = alter_disp_name_all($result);
  }

  return $result;
}Code language: plaintext (plaintext)

変数 $disp_mode が 0なら、$resultには何もしない、

0でないなら、$resultに何か変更を加える（表示調整する）、

ことがわかりやすくなりました。

for文の中身を別メソッドに分ける

for文の中身だけをさらに別メソッドに分けます。

分けたメソッド内では、for文で回してもいいですし、foreach文でもいいですし、array_map関数を使ってもいいと思います。

function alter_disp_name_all($result_arr){
  for ($i=0;$i<count($result_arr);$i++){
    $result[$i] = alter_disp_name($result[$i]);
  }
  return $result;
}

function alter_disp_name($rcd) {
  $pos = strpos($rcd['name'],"CLASS_CD_");
  if ($pos === false){
  } else {
    //区分CDの場合('CD'が最後に現れる箇所)
    // $rcd['disp_name']はシステム側で自動生成 末尾が「CD」の文字列か空文字列
    $pos = strrpos ($rcd['disp_name'],"CD");
    if ($pos === false){
    } else {
      //文字列CDが有る場合
      $f_name = substr($rcd['disp_name'], 0, $pos);
      if (mb_strlen($f_name) > 4){
        //4文字以上の場合は5文字目以後を切り落とし
        $f_name = mb_substr($f_name, 0, 4) . "..CD";
      } else {
        if ($f_name != ''){
          $f_name .= 'CD';
        } else {
          $f_name .= '　';
        }
      }
      //上書き
      $rcd['disp_name'] = $f_name;
    }
  }
  return $rcd;
}Code language: plaintext (plaintext)

配列全体への処理と、配列要素への個別処理は、分けたほうがわかりやすいわね

表示用調整にいたるまでの条件は、2種類あるようです。

#条件
1$rcd['name']が'CLASS_CD_"始まりかどうか
2$rcd['disp_name']が"CD"終わりかどうか

$f_name の表示用調整は、4種類あることがわかってきました。

#処理
0何もしない
14文字以上の場合は5文字目以後を切り落として、"..CD"を追加する
2'CD'を追加する
3'　'を追加する

よく見ると、コメントにいろいろ書いていますね

//区分CDの場合('CD'が最後に現れる箇所)
//末尾が「CD」の文字列か空文字列
//4文字以上の場合は5文字目以後を切り落としCode language: plaintext (plaintext)

架空の言語で書くと、次のようなロジックでした。

if (区分CD) {
  if (末尾に「CD」がある) {
    if (5文字以上) {
        // 処理1:4文字以上の場合は5文字目以後を切り落として、"..CD"を追加する
    } elseif (1文字以上) {
        // 処理2:'CD'を追加する
    } else {
        // 0文字、つまり空文字なら
        // 処理3:'　'にする
    }
  }
}Code language: plaintext (plaintext)

ver2

理解が難しいロジックは、決定表を作って整理します。

条件1〜2、処理1〜3は、ver1でまとめた条件と処理です。

#$rcd
[’name']		(条件1)
区分CDか？		$rcd
['disp_name']
(条件2)
'CD'終わり		名称部分
の長さ		処理変更後
1'not_cd_fuga'No何もしない
2'CLASS_CD_fuga'Yes''No何もしない''
3↑Yes'fugafugaCD'Yes>4処理1'fuga..CD'
4↑Yes'fugaCD'Yes1〜4処理2'fugaCD'
5↑Yes'CD'Yes0処理3'　'

わかりやすいし、表が正しいかどうか検証しやすいわね

決定表を書かなくても、変更前と変更後の例があるだけで、わかりやすくなるよね

変更前と変更後の例を書いていると、いつのまにか決定表を書いていることもあるわ

処理3（全角スペースに置換する）の意図がよくわかりませんが、元のコードを信じることにします。

この決定表を書いているうちに、$rcd['disp_name']が''ではなく、'CD'終わりでもない、'fugafugaXY'のよなケースを思いつきました。

コード内のコメントや元記事のコメント欄を読むと $rcd['disp_name']は、'' または 'fugafugaCD' のどちらかしかないそうです。これはケース番号 #2の「'CD'終わりではない」と判別していいようです。

#$rcd
[’name']		区分CDか？$rcd
['disp_name']
'CD'終わり		名称部分
の長さ		処理変更後
2↑Yes'fugafugaXY'No

さきほどのコードに、このケース番号を書き入れます。

/*
-------------------------------------------------------------------------------------
# $rcd['name']    区分CD  $rcd['disp_name'] 'CD'終わり 名称部分の長さ 処理       変更後
-------------------------------------------------------------------------------------
1 'not_cd_fuga'   No     ーーー            ーーー     ーーー         何もしない
2 'CLASS_CD_fuga' Yes	 ''                No         ーーー         何もしない ''
3 ↑              Yes    'fugafugaCD'      Yes        >4             処理1      'fuga..CD'
4 ↑              Yes    'fugaCD'          Yes        1〜4           処理2      'fugaCD'
5 ↑              Yes    'CD'              Yes        0              処理3      '　'
-------------------------------------------------------------------------------------
*/
function alter_disp_name($rcd) {
  $pos = strpos($rcd['name'],"CLASS_CD_");
  if ($pos === false){
    // #1
  } else {
    $pos = strrpos ($rcd['disp_name'],"CD");
    if ($pos === false){
      // #2
    } else {
      $f_name = substr($rcd['disp_name'], 0, $pos);
      if (mb_strlen($f_name) > 4){
        // #3
        $f_name = mb_substr($f_name, 0, 4) . "..CD";
      } else {
        if ($f_name != ''){
          // #4
          $f_name .= 'CD';
        } else {
          // #5
          $f_name .= '　';
        }
      }
      //上書き
      $rcd['disp_name'] = $f_name;
    }
  }
  return $rcd;
}Code language: plaintext (plaintext)

一見、不要に見えた２箇所の if ($pos === false) にケース番号「#1」「#2」がぴったりはまりました。元のコードを書いた人は、決定表を書いてから、コードに落とし込んだのかもしれません。

えー、コードから決定表を作ったからじゃないの？

そうかもしれないわね

決定表のケース番号があるなら、このままでもコードを追跡できます。これ以上リファクタしなくてもいいと思います。

for文の中身をメソッドに分けただけだよね

確かに、元のコードはそんなにひどくなかったみたいね

ver3

せっかくの投稿記事なんだし、やれるところまでリファクタしてみたら

表示用調整ををする／しないの判断と、表示用調整を２つに分けたらどうでしょうか。

まず、決定表を２つに分けます。

１つめの決定表は、表示用調整をする／しないです。決定表は不要なくらいシンプルです。

#$rcd
[’name']		区分CDか？$rcd
['disp_name']
'CD'終わり		表示用調整
1-1'not_cd_fuga'Noしない
1-2'CLASS_CD_fuga'Yes''Noしない
1-3'CLASS_CD_fuga'Yes'fugafugaCD'Yesする

２つめの決定表は、表示用調整です。

#$rcd
['disp_name']
'CD'終わり		名称部分
の長さ		処理変更後
2-1'fugafugaCD'Yes>4処理1'fuga..CD'
2-2'fugaCD'Yes1〜4処理2'fugaCD'
2-3'CD'Yes0処理3'　'

これにしたがって、2つのメソッドを呼ぶようにします。

元々のコメントを残しておきます。

// 区分CDの場合('CD'が最後に現れる箇所)
// 末尾が「CD」の文字列か空文字列
// 4文字以上の場合は5文字目以後を切り落とし

function alter_disp_name($rcd) {
  if (is_cd($rcd)) {
    $rcd['disp_name'] = shorten_disp_name($rcd['disp_name']);
  }

  return $rcd;
}
Code language: plaintext (plaintext)

１つめの決定表のメソッドです。

/*
--------------------------------------------------------------------------------
#  $rcd['name']      区分CDか  $rcd['disp_name']  'CD'終わり 表示用調整
--------------------------------------------------------------------------------
1-1 'not_cd_fuga'    No        ---                ---         No
1-2 'CLASS_CD_fuga'  Yes       ''                 No          No
1-3 'CLASS_CD_fuga'  Yes       'fugafugaCD'       Yes         Yes
--------------------------------------------------------------------------------
*/
function is_cd($rcd) {
  $class_cd_pos = strpos($rcd['name'],"CLASS_CD_");
  $cd_pos = strrpos ($rcd['disp_name'],"CD");

  if ($class_cd_pos === false){
    // #1-1
    return false;
  } elseif ($cd_pos === false){
    // #1-2
    return false;
  } else {
    // #1-3
    return true;
  }
}
Code language: plaintext (plaintext)

２つめの決定表のメソッドです。

このメソッドは、引数$disp_nameが'CD'で終わっていることを前提にしています。'CD'終わりではないとき、例外を投げても不自然ではありません。

つまり「'CD'終わりかどうかの判定」はガード節なので、アーリーリターンします。

/*
------------------------------------------------------------------------
#   $disp_name     'CD'終わり 名称部分の長さ  処理   変更後
------------------------------------------------------------------------
2-1 'fugafugaCD'   Yes         >4             処理1  'fuga..CD'
2-2 'fugaCD'       Yes         1〜4           処理2  'fugaCD'
2-3 'CD'           Yes         0              処理3  '　'
------------------------------------------------------------------------
*/
function shorten_disp_name_str($disp_name) {
  $pos = strrpos($disp_name,"CD");
  if ($pos === false) {
    // throw new Execption();   // 仮に、例外を投げるように書いても不自然ではない
    return $disp_name;
  }

  $f_name = substr($disp_name, 0, $pos);
  if (mb_strlen($f_name) > 4){
    // #2-1
    $disp_name = mb_substr($f_name, 0, 4) . "..CD";
  } elseif ($f_name != ''){
    // #2-2
    $disp_name = $f_name . 'CD';
  } else {
    // #2-3
    $disp_name = '　';
  }
  return $disp_name;
}Code language: plaintext (plaintext)

is_cd()、shorten_disp_name() どちらのメソッドでも、disp_nameの'CD'終わりかどうかを判定していることが気になります。

ver4

ここまでにわかったことを整理して、架空の言語で書くと、

if ($rcd['name']が CLASS_CD_ 始まり) {
  if ($rcd['disp_name']が'CD'終わり) {
    $rcd['disp_name']の表示用調整
  }
}
Code language: plaintext (plaintext)

ver3では、「$rcd['name']が 'CLASS_CD_'始まり」と「$rcd['disp_name']が'CD'終わり」を一つのメソッドにしました。

ver4では、「$rcd['disp_name']が'CD'終わり」と「$rcd['disp_name']の表示用調整」を一つのメソッドにしてみましょう。

まず、alter_disp_nameメソッドは、ver3と同じです。

function alter_disp_name($rcd) {
  if (is_cd($rcd)) {
    $rcd['disp_name'] = shorten_disp_name($rcd['disp_name']);
  }

  return $rcd;
}
Code language: plaintext (plaintext)

次に、$rcd['name']が'CLASS_CD_'始まりかどうかの判定メソッドです。決定表は不要ですね。

function is_cd($rcd) {
    $pos = strpos($rcd['name'],"CLASS_CD_");
    return $pos !== false;
}
Code language: plaintext (plaintext)

そして、表示用調整メソッドです。

決定表に、$disp_nameが ''かどうかを追加して、ケース番号をふり直しました。

さて、ケース番号 #1をアーリーリターンにするか、else でつなげていくか考えます。さきほどはアーリターンしました。アーリーリターンのほうが読みやすいですし、後半のネストも浅くなります。

しかし、このメソッドは、$disp_nameが''の場合も想定しています。この場合に例外を投げたら、それは間違いです。これはガード節ではありません。

そこで、elseでつなげるほうを選びました。

/*
------------------------------------------------------------------------
#   $disp_name     'CD'終わり  名称部分の長さ  処理       変更後
------------------------------------------------------------------------
1  ''             No             ーーー　　　  何もしない  ''
2  'fugafugaCD'   Yes            >4            処理1       'fuga..CD'
3  'fugaCD'       Yes            1〜4          処理2       'fugaCD'
4  'CD'           Yes            0             処理3       '　'
------------------------------------------------------------------------
*/
function shorten_disp_name($disp_name) {
  $pos = strrpos($disp_name,"CD");
  if ($pos === false) {
    // #1
    $out_disp_name = $disp_name;
    // throw new Exception();   // 仮に、ここで例外を投げたら間違い、ガード節ではない
  } else {
    $f_name = substr($disp_name, 0, $pos);
    if (mb_strlen($f_name) > 4){
      // #2
      $out_disp_name = mb_substr($f_name, 0, 4) . "..CD";
    } elseif ($f_name != ''){
      // #3
      $out_disp_name = $f_name . 'CD';
    } else {
      // #4
      $out_disp_name = '　';
    }
  }

  return $out_disp_name;
}Code language: plaintext (plaintext)

ケース番号 #1 でアーリーリターンする、しないは別としても、まだすっきり感が足りません。

それは、if 〜 else を進みながら、その途中で、条件を判断するための値を取得しているからです。

$f_name = substr($disp_name, 0, $pos);
if (mb_strlen($f_name) > 4){

判断のもととなる値は、可能なら最初に収集してしまってから、判断するようにすると、

  • ifの条件式がシンプルになります。
  • elseif でつなげられるので、ネストが深くなりません。
/*
------------------------------------------------------------------------
#   $disp_name   'CD'終わり  名称部分の長さ  処理   変更後
------------------------------------------------------------------------
1  ''             No          ーーー　　　　 何もしない  ''
2  'fugafugaCD'   Yes         >4              処理1      'fuga..CD'
3  'fugaCD'       Yes         1〜4            処理2      'fugaCD'
4  'CD'           Yes         0               処理3      '　'
------------------------------------------------------------------------
*/
function shorten_disp_name($disp_name) {
  $pos    = strrpos($disp_name,"CD");
  $f_name = substr($disp_name, 0, intval($pos));
  $len    = mb_strlen($f_name);

  if ($pos === false) {
    // #1
    $out_disp_name = $disp_name;
  } elseif ($len > 4) {
    // #2
    $out_disp_name = mb_substr($f_name, 0, 4) . "..CD";
  } elseif ($len > 0){
    // #3
    $out_disp_name = $f_name . 'CD';
  } else {
    // #4
    $out_disp_name = '　';
  }

  return $out_disp_name;
}Code language: plaintext (plaintext)

まとめ

最終的なコードです。

function get_tbl_data_mapping_info($db, $table_id, $disp_mode=0, $org_no=null){
  //レコード取得
  $result = select_tbl_data_mapping_info($db, $table_id, $org_no);

  if ($disp_mode != 0){
    $result = alter_disp_name_all($result);
  }

  return $result;
}

function alter_disp_name_all($result_arr){
  for ($i=0;$i<count($result_arr);$i++){
    $result[$i] = alter_disp_name($result[$i]);
  }
  return $result;
}

function alter_disp_name($rcd) {
  if (is_cd($rcd)) {
    $rcd['disp_name'] = shorten_disp_name($rcd['disp_name']);
  }

  return $rcd;
}

function is_cd($rcd) {
    $pos = strpos($rcd['name'],"CLASS_CD_");
    return $pos !== false;
}

/*
------------------------------------------------------------------------
#   $disp_name   'CD'終わり 名称部分の長さ  処理   変更後
------------------------------------------------------------------------
1  ''             No         ーーー　　　　　何もしない  ''
2  'fugafugaCD'   Yes        >4              処理1      'fuga..CD'
3  'fugaCD'       Yes        1〜4            処理2      'fugaCD'
4  'CD'           Yes        0               処理3      '　'
------------------------------------------------------------------------
*/
function shorten_disp_name($disp_name) {
  $pos    = strrpos($disp_name,"CD");
  $f_name = substr($disp_name, 0, intval($pos));
  $len    = mb_strlen($f_name);

  if ($pos === false) {
    // #1
    $out_disp_name = $disp_name;
  } elseif ($len > 4) {
    // #2
    $out_disp_name = mb_substr($f_name, 0, 4) . "..CD";
  } elseif ($len > 0){
    // #3
    $out_disp_name = $f_name . 'CD';
  } else {
    // #4
    $out_disp_name = '　';
  }

  return $out_disp_name;
}Code language: plaintext (plaintext)

メソッドに分けていった結果、ネストも浅くなりました。元のコードと比較して読みやすくなったかというと微妙な感じもします。

しかし、決定表を作ったことで、表示用調整のコードを追跡しやすくなり、テストケースを書きやすくなりました。

リダーブルコードではアーリーリターンを推奨しているけど、アーリーリターン（早期リターン）を使わないの？と思った人向けに記事を書きました。

早期リターンとApple SLLバグ
早期リターン（アーリーリターン）とは、リダーブルコードやその他の本で推奨されている書き方です。ガード節でnullチェックや範囲チェックして、想定外の値のときは、すぐにリターンします。else で続けません。 function foo() {...
www.ninton.co.jp
2021.04.26
タイトルとURLをコピーしました