リファクタリングを考える時期の記事紹介
自分の英語力アップと、技術力アップを兼ねて、好きな開発者の書いた記事をある程度翻訳して載せていこうと思います。今までMarkとかMcurryとかの興味深い記事をざっとは読んでたけど、自分が読むだけだと流し読みになりやすくて、読んだつもりになっただけで何も残らず、いかんなぁと思ってました。
全訳ではなく、自分が記事を理解して、ざっくりと翻訳して発信していくようにすれば、まず記事の内容を理解し、それを分かりやすく短く書くようになるので、良いのではないかと思いました。一人勉強会みたいなもんです。
これを続けていき、ある程度の段階になったら自分も英語で定期的に発信していきたい。
mark-storyの記事
5 signals that can indicate its time to re-factor
リファクタリングすべき時期はいつごろか?その指標となる5項目を挙げています。
1. メソッドのパラメータが多くなった時 2. ひとつのメソッドが多機能になってしまった時 3. 同じようなコードがいくつも存在している時 4. 名前の付け方が悪い時 5. あるべき場所(階層)にあるべきコードを書く
では、簡単に紹介。
1. メソッドのパラメータが多くなった時
パラメータが多くなると、パラメータや順序を覚えてなきゃいけないので、ミスが多くなる。
Cake1.1と1.2のfindメソッドで比較してみよう
<?php //Cake1.1の場合 $this->Post->findAll($conditions, $fields, $order, $limit, $page, $recursive); //Cake1.2の場合 $this->Post->find($type, $options); ?>
Cake1.1の場合はパラメータ6個!!で順番覚えるのも大変。
Cake1.2ののModel::find()の例は、必要な情報をoptionsに連想配列渡しするので、順番とか覚えてなくてもいいという例。
市川補足
Cake1.2ののModel::find()のパラメータ例は、model::findのコメント欄で書いている用法。実際のAPIマニュアルとは違う。
/** * Returns a result set array. * * Also used to perform new-notation finds, where the first argument is type of find operation to perform * (all / first / count / neighbors / list / threaded ), * second parameter options for finding ( indexed array, including: 'conditions', 'limit', * 'recursive', 'page', 'fields', 'offset', 'order') * * Eg: find('all', array( * 'conditions' => array('name' => 'Thomas Anderson'), * 'fields' => array('name', 'email'), * 'order' => 'field3 DESC', * 'recursive' => 2, * 'group' => 'type')); */
2. ひとつのメソッドが多機能になってしまった時
メソッドに2つ以上の機能を入れてしまうと、テストがしにくいことと、使う場合に迷ってしまうことがある。
もし自分の作ったメソッドを説明する時に、and, or , butという言葉が入るならメソッドを2つ以上に分けたほうが良い。
<?php public function getNode($id = null, $addView = true){ $conditions = array(); if (is_numeric($id)) { $conditions['Node.id'] = $id; } elseif(is_string($id)) { $conditions['Node.slug'] = $id; } $node = $this->nodeInfo($conditions); if ($addView && !empty($node)) { $this->id = $node['Node']['id']; $this->saveField('views', ($node['Node']['views'] +1)); } return $node; } ?>
上記の例だとgetNodeという名前なのに、後半でViewのカウントをインクリメントしている。
この場合は、次のように、viewカウントをインクリメントする機能を別メソッドに切り出したほうが良い。
<?php public function getNode($id){ $conditions = array(); if (is_numeric($id)) { $conditions['Node.id'] = $id; } else { $conditions['Node.slug'] = $id; } return $this->nodeInfo($conditions); } public function updateViewCount($id) { return $this->updateAll( array('Node.views' => 'Node.views + 1'), array('Node.id' => $id) ); } ?>
こうするとテストもしやすくなるし、メソッド名から処理内容が分かるし、コードが読みやすくなる。
3. コードが重複している時
アップデートし忘れがあるので、重複コードは排除した方が良い。
元記事の例では、渡すパラメータの値が違うだけなので、それぞれのパラメータを事前に配列でセットして、foreachで回している例が載ってます。
4. 名前の付け方が悪い時
あまり変数名とか略しすぎないほうが良いとか。名前は重要。
市川感想
誰でもわかりやすくて的確な機能をもつ名前、変数をつけるのは、日本人にとっては英語力が結構いる気がする。分かりやすくても長い名前にするとスペルミスとかするし(エディタ補完が必須になる)。
5. あるべき場所(階層)にあるべきコードを書く(超勝手に訳しましたw)
オリジナルのタイトルはMixed levels of abstraction and muddled areas of concernです。
あるべき階層に、あるべきコードを書く。考える範囲を超えてコードをごちゃまぜにしない。例えばコントローラ内でSQL文を書くとかしない。SQL文はモデルに入れて、それをコントローラから呼び出すようにする。
まとめ
リファクタリングしてよりシンプルなコードに、テストしやすく、メンテナンスが楽になるのが理想。