Do You PHP はてブロ

Do You PHPはてなからはてブロに移動しました

「リファクタリングが必要」という兆候

PHP 雑記

リファクタリングはPHPに限った話ではないですが、PHPを使っている場合に「どういった兆候が現れるとリファクタリングした方が良いか」といった話がまとめられています。


I have had to go through a php application recently which has given me more than one headache and has required me to use all my possible patience.

あまり目新しいものはないかも知れませんが、以下要点を訳してざっとまとめてみました。間違いがあれば指摘してください :-)

グローバル変数

大量のブラケット

  • 以下のようなコード
<?php
$variable['field']['subfield']['subsubfield1'] = 'value1';
$variable['field']['subfield']['subsubfield2'] = 'value2';
$variable['field']['subfield']['subsubfield3'] = 'value3';
$variable['field']['subfield']['subsubfield4'] = 'value4';
...
    • 配列をうまく使う
<?php
$variable['field']['subfield'] =
  array('subfield1'=>'value1', 'subfield2'=>'value2', 'subfield3'=>'value3', 'subfield4'=>'value4');
  • 違う例
<?php
$prop1 = $res['results'][0]['property1']
$prop2 = $res['results'][0]['property2']
...
    • 改善案はこんな感じ以下のコードは、上記配列に対して動作しません。追記を参照してください。
<?php
list($prop1, $prop2...) = array_shift($res['results']);

全て配列にしている

  • 全てpublic扱いになってしまうので、オブジェクトを使おう
    • まあ、PHP5の話だということで。。。

コードの重複

  • コピペじゃなくて、関数化する

終わりのないswitch文

  • 300行のswitch文とか、パッと見て何やってるのか分からないようなコード
  • 適宜関数化する

非常に多くのファイルを修正する必要がある

  • 設計の問題。過度の密結合

ハードコードされた値

  • 定数を使うべき

インターフェースの不一致

  • 戻り値がまちまち
    • たとえば、処理の成否を返す場合、booleanを返したり、0/1を返したりしている
    • 戻り値は一定させる

追記（2007/06/06 16:40）

上にあるlist関数を使ったサンプルですが、正しく動作しません。亀本さん＠アシアル、ありがとうございます〜 :-)
list関数は、「数値添字の配列」でのみ使用できます（http://jp.php.net/listにあるmysql_fetch_rowを使ったサンプルを参照）。上記の場合、$resが以下のような配列であれば、list関数が改善策になり得ます。

<?php
/**
 * $prop1 = $res['results'][0][0];
 * $prop2 = $res['results'][0][1];
 * ...
 */
$res = array();
$res['results'][] = array('prop1',
                          'prop2',
                          'prop3',
                          'prop4',
                          'prop5');
$res['results'][] = array('prop6',
                          'prop7',
                          'prop8',
                          'prop9',
                          'prop10');
while (list($prop1, $prop2, $prop3, $prop4, $prop5) = array_shift($res['results'])) {
    var_dump($prop1);
    var_dump($prop2);
    var_dump($prop3);
    var_dump($prop4);
    var_dump($prop5);
}

なお、連想配列の場合、extract関数が使えますが、取り扱い注意ということで。

<?php
$res = array();
$res['results'][] = array('property1' => 'prop1',
                          'property2' => 'prop2',
                          'property3' => 'prop3',
                          'property4' => 'prop4',
                          'property5' => 'prop5');
$res['results'][] = array('property1' => 'prop6',
                          'property2' => 'prop7',
                          'property3' => 'prop8',
                          'property4' => 'prop9',
                          'property5' => 'prop10');

// foreach ($res['results'] as $result) {
//     extract($result);
// でも良いけど、とりあえず1行で。。。「@」を書かないですむ方法ないかなぁ
while (@extract(array_shift($res['results']))) {
    var_dump($property1);
    var_dump($property2);
    var_dump($property3);
    var_dump($property4);
    var_dump($property5);
}