読者です 読者をやめる 読者になる 読者になる

woshidan's blog

そんなことよりコードにダイブ。

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

見つけて時間がありしだい少しずつ直すんですけど、全然直る気がしません。

 

少なくとも最初っから地雷パターンがあるような気がしたので、

(そして地雷になりそうで、後半書き出すのが少し苦しくなったので)

今回特にひどかったことを戒めとして恥をさらしておきます。

 

機能に必要な動詞、ちゃんと網羅してる?

「期待される値を入力した配列とテスト対象のファイル」を受け取って、

「テスト対象のファイルに保存されている値が期待される値かをチェックして正しくなければ、正しい値とエラーが出た旨を出力する」

という機能を持つクラスを書いていたのだけど、その機能を

 

・期待される値を計算する関数

・保存されている値を抽出する関数

・正しい値が付属されたエラーメッセージを生成する関数

・期待されている値と入力されている値を比較する関数

 

に分けていた。上二つも問題なのだけど、

何がひどいかというと、これ、

・期待される値と保存される値の取得形式を揃える

・比較結果がエラーかどうか判別する(エラーなのか、例外なのか)

・エラーメッセージを受け取る

部分が入ってなかったことがコードを書き出してから分かった。

このしわ寄せが一番下に来てしまっていて、

一番下は実際には、

「期待されている値と入力されている値を比較して、

 二つの値の形式に例外があったら対応をし、

 エラーがあったらエラーの内容を調べて出力する関数」

になってしまいました。

 

最初はそれでいいじゃん! if文くらい書いても!

エラー処理の呼び出し一行くらい余分に書いても!と思ったのですが、

実際にはif文だけで終わらず、例外かもしれない場合の処理であるとか、

エラーメッセージを受け取るための処理とかが入って来て

出来上がってみたらくそコードになっていたのでした。

 

たぶん、

・期待されている値と入力されている値を比較する関数

・二つの値の形式に例外があったら対応をする関数

・エラーがあったらエラーの内容を調べる関数

・エラーの内容に応じて出力する関数

・上4つを使う関数

に分ければよかった気がします。

 

それ10行で書ける当てがあるの?(間の動詞抜けてない?)

〜〜して○○する、という言い方になってない?

というのを一つの基準にしよ……。

 

比較対象の2つの値への処理は対称になっているか?

あと、

・期待される値を計算する関数

・入力されている値を抽出する関数

処理内容がいまいち対応していない(その結果、結構上か下にあわせてもう一つ余計に変換処理書くことが多かった)のも問題で、もう少し具体的にして

・入力された期待される値から比較用の期待される値を計算する関数

期待される値の比較用の配列を作る関数

・テスト対象のファイルから保存されている値を抽出する関数

・抽出した値から保存されている値の比較用の配列を作る関数

とせめて保存されている値、期待される値の取得・加工処理の最終的な出力形式をきっちり揃えるべきでした。

そうしておかなかったので、値を加工する処理が比較をする関数に入って来て読みにくいしテストしにくかったです……。

 

部分的に汚く見える部分があったら、その部分と同じ階層の値全部を関数で包む

配列に計算した値を入れていくとき、後から繰り返し使う部分じゃなかったら、

以下のように一部に複雑な計算をする要素が入ってもわざわざ関数にする必要がないかな、と思っていたのだけど、一晩置いたら想像以上に読みにくかったです。

大体こんな感じでした。

function getCorrectValues(test_part){
  correct_values = array(
      document_values['title'],
      document_values['title']
      + " | "
      + document_values['description']
      + " | "
      + document_values['values'],
      .....,

      );
  return correct_values;
}

こんな感じで、部分的に複雑になっている場合は、

全部同じくらい単純に見えるように関数で包んであげると気が楽。

function getCorrectValues(){
  correct_values = array(
      makeCorrectValue('title'),
      makeCorrectValue('head'),
      .....,

      );
  return correct_values;
}

function makeCorrectValue(part){
  switch(part){
    case 'title':
      correct_value = document_values['title'];
      break;
    case 'head':
      correct_value =  document_values['title']
                    + " | "
                    + document_values['description']
                    + " | "
                    + document_values['values'];;
      break;
    ......
  }
  return correct_value;
}

 

これだけだと何が違うんだ、という感じで直接書けばいいじゃないか、

特に単に値だけ書いている部分は関数を呼ばなくても良いじゃないか、

という当初の自分の考え。

 

甘かった(笑)

 

だんだん、一つの値から使い回す部分が増えたり、

特定の要素の数が可変で、その数に合わせて期待されている値の配列の長さが変わるので

ループを書くようになったり、例外的に値を計算するためのif文が必要になったり……、

ということがありました。

 

そういう場合こうしておくととてもコードを読むのが楽になったかなぁ、と思った……。

あと、switch文の分岐(配列でもなんでも良いかも?)の数と列が一対一で対応しているとどれを書いてどれを書いてないっけ……と考えなくて良いので、書くのが比較的楽。

単純な部分があるとその部分は別に良いかな? と思ってしまうけど、

同じ階層の値は同じ見た目になっていないと読みにくいというか、自分が読む気が失せることが分かりました。

 

それに、どの値も急な仕様変更が入ったりして複雑になりえますしねー……。