続・関数の書き方の悪癖について

来るべき案件に備えて勉強をしつつ、

悪いインターンなのでテストが終わっている内製ツールをまだ手直ししたりします。

それで、昨日ちょっと思った事。

クラス名など関数や変数の外側で何をさしているか分かる場合、命名を特殊なものにしない

命名は基本的に具体的なものほどよいと思っているのですが、

複数のクラスで似たような処理があって、クラス名を見たら何を扱うか分かる場合は

同じ名前だから同じ内容の処理だよー、というメッセージの方を重視して、

名前を少し抽象的なものに揃えて置いた方が読みやすい気がします。

class DivTest extends Test
{
  function buildCorrectDivSample($correct_div_data)
  {
    //正解のdivのソースコードを返します
  }

  function buildDivTestPattrn($div_contents)
  {
    //divの中身を抜き出すための正規表現を返します
  }
}

class ListTest extends Test
{
  function buildCorrectListSample($correct_data)
  {
    //正解のリストのソースコードを返します
  }

  function buildListTestPattrn($list_items)
  {
    //リストの中身を抜き出すための正規表現を返します
  }
}

よりも、

class DivTest extends Test
{
  function buildCorrectSample($correct_data)
  {
    //正解のdivのソースコードを返します
  }

  function buildtTestPattrn($contents)
  {
    //divの中身を抜き出すための正規表現を返します
  }
}

class ListTest extends Test
{
  function buildCorrectSample($correct_data)
  {
    //正解のリストのソースコードを返します
  }

  function buildtTestPattrn($contents)
  {
    //リストの中身を抜き出すための正規表現を返します
  }
}

にしておいたら、同じ処理なんだな感が、出て、

しかも外から呼び出すときにいちいちメソッドの名前を確認する必要がなくなる。

で、あと、

class Test
{
  abstract function buildCorrectSample($correct_data);
  abstract function buildtTestPattrn($contents);
}

にしておくと、表記にゆらぎが出た場合エラーを吐いてくれます。

データベースではxx_nameみたいな列を作るな、という感じです。

それにしても、buildTestPatternよりたぶん、buildExtractDataPatternのほうがいいですよね……。

あと最初からきちんと機能把握して抽象メソッド書けやという話ですね。

本筋以外が目立つ場合も関数に抜き出す

関数を書くとき自分は、

function executeTest($test_data)
{
 #テスト対象のファイルを取ってくる
 #テスト対象のファイルのコードからテスト対象の部分だけ抜き出す
 #ファイルによってテストの形式が異なるので判定する
 #テストを行う
 #値が正しくなかったらエラーメッセージを吐く
 #エラーメッセージを返す
}

みたいに大雑把に処理を日本語で書くんですね。

主語目的語がそろってなくてなかなかあれなんですが。とりあえず。

で、その後、

function executeTest($test_data)
{
 $file_source = getTargetFile($test_data['file_path']);
 $test_source = getTargetSource($file_source);
 
 #ファイルによってテストの形式が異なるので判定する
 preg_match('/(div|list)/',$test_data['file_path'], $matched_result);
 $test_name = matched_result[1];
 
 if($test_name === 'div')
 {
    $error = DivTest($test_source);
 } else if($test_name === 'list') {
    $error = ListTest($test_source);
 } else {
    print "ファイルの名前がおかしい\n";
    return false;
 }
 
 return $error;
}

で、大体関数にまとめるんですけど、

呼び出しているテスト用の関数名が見たいしそこまで行数もないし、

ってことでテストの種類の判定部分は全部書いちゃうんですね。

でも、この状態だと、

この関数はテストの流れ全体を扱っている関数なのに、

テストの種類の判定の部分が表示割合として大きく見えちゃうんですね。

いまいち。

というか、どちらかというと、

テストの種類がファイルで分岐するよ、という部分こそが

大事だと思って残しているので分岐が残っている事自体は問題じゃなくて、

preg_matchの行が処理の分岐とは違う流れの処理を連想させるから読みにくい気がします。

なので、preg_matchのところの二行をいかにも軽い処理ですよ風な名前の関数で抜き出して、すっきりさせてみる。

function executeTest($test_data)
{
 $file_source = getTargetFile($test_data['file_path']);
 $test_source = getTargetSource($file_source);
 
 #ファイルによってテストの形式が異なるので判定する
 $test_name = getTestName($test_data['file_path']);
 if($test_name === 'div')
 {
    $error = DivTest($test_source);
 } else if($test_name === 'list') {
    $error = ListTest($test_source);
 } else {
    print "ファイルの名前がおかしい\n";
    return false;
 }
 
 return $error;
}

function getTestName($file_path)
{
  preg_match('/(div|list)/',$test_data['file_path'], $matched_result);
  return $matched_result[1];
}

あるいは、こう。

分岐の結果がリストにしろ、divにしろ、

どちらにせよhtmlの要素の値についてテストしたいんだから、

両方をまとめた名前で判別部分ごと関数にまとめてみる。

function executeTest($test_data)
{
 $file_source = getTargetFile($test_data['file_path']);
 $test_source = getTargetSource($file_source);
 
 $error = ElementTest($test_data, $test_source);
 if($error == false)
 {
    print "ファイルの名前がおかしい\n";
 }

 return $error;
}

function ElementTest($test_data, $test_source)
{
  #ファイルによってテストの形式が異なるので判定する
  preg_match('/(div|list)/',$test_data['file_path'], $matched_result);
  $test_name = $matched_result[1];
 
  if($test_name === 'div')
  {
    $error = DivTest($test_source);
  } else if($test_name === 'list') {
    $error = ListTest($test_source);
  } else {
    return false;
  }
  return $error;
}

一瞬、

そうだ！ファイルを取り出すところから抜き出せば良いのでは、

と思いましたが、それは元の木阿弥ですよねー。

どちらの方針にするかはどこの部分がややこしいかによって変える感じです。

読みにくくなってるのか、読みやすくなってるのか分からないけど、

テストの種類の判定、あるいはテストの名前の取得っていう一つの機能を抜き出していることになるから個人的にはOK。

本筋以外が長くなったら、というより、

判定の部分も一つの関数に分けた方が(自分にとっては)読みやすいのかもしれないです。

とりあえず複雑になっても複雑な部分が短かったら読んでいけるので、関数に分ける気持ちでいきます。

意味もなく分けすぎるとコードが散る事による弊害が起こるのでよくない(たとえば上の二つを併用するとか)気がします。

いまさらなんですが、その場で適当に考えた例なので、二つの部分の関数は関連がありそうでないです。

他は、「Stateパターンを使ってみたーい」ということで(手段と目的が逆だし違う気しかしない)、同じ要素についてはサンプルコードとテストデータ抜き出しのパターンを縦に並べて見れるようにしてみたりしてました。