はじめに
本投稿は、2015/10/23に行われた、リファクタリング（サーバー編） - connpassの内容についてまとめた資料です。
今後の予定は以下に掲載されますのでよろしくお願いします！
AWS上で構築するRESTfulアプリ勉強会～Web開発ワークショップ～ - connpass
今回は、「リファクタリング」について学びます。
リファクタリングとは?
プログラムの動作を変えずに内部構造を改善すること。ようするに、コードをキレイにするということ。
リファクタリング手順
- リファクタリングすべきか判断する。
- テストがあるか？なければ書く。
- リファクタリングする
- テストする
- 1に戻る
リファクタリングすべきか判断する - コードの匂い
コードの「不吉な匂い」。リファクタリングすべきコードは、匂いを発します。
以下の様なコードは、匂います!
引用:不吉な匂い
- 重複したコード
- 長すぎるメソッド
- 巨大なクラス
- 多すぎる引数
- 変更の発散
- 変更の分散
- 属性、操作の横恋慕
- データの群れ
- 基本データ型への執着
- スイッチ文
- パラレル継承
- 怠け者クラス
- 疑わしき一般化
- 一時的属性
- メッセージの連鎖
- 仲介人
- 不適切な関係
- クラスのインタフェース不一致
- 未熟なクラスライブラリ
- データクラス
- 相続拒否
- コメント
分類してみました
ムリヤリ感と基準がよくわからない感がありますが、ちょっとだけとっつきやすくなるよう、分類してみました
- 激臭系
- 蓋を開けたら臭う系
- メンドクサイ系
- 加齢臭系
- 生臭い系
- 汗臭い系
- ウソ臭い系
以下、解説します。
各臭い毎に、「消臭方法」や「〜原則に違反」などを書いてますが、ここで説明するには紙面が足りません
興味あればググってみてください。
激臭系
問答無用で臭い。
読まずとも「眺める」だけで匂うコード。
-
- 長すぎるメソッド
- 分割しよう。
-
- 巨大なクラス
- 分割しよう。
-
- 多すぎる引数
- 分類してオブジェクトにしよう。
-
- スイッチ文
- オブジェクト指向しましょう。
蓋を開けたら臭う系
少し読んでみたらすぐ臭さに気づくレベルの臭い。
-
- 重複したコード
- DRY原則違反。 コピペやめて共通化しましょう。
-
- 怠け者クラス
- やってることが少なくて存在意義の薄いクラス。他のクラスに役割を吸収させましょう。
-
- 仲介人
- 自分では何もしていないクラス。削除もしくはもっと仕事させましょう。
-
- クラスのインタフェース不一致
- 別の担当者が同じ物作った？意識合わせしましょう。
-
- データクラス
- いわゆる「ドメインモデル貧血症」。データと振る舞いをもたせましょう。
メンドクサイ系
くさいの意味がちょっと違うのでカタカナ(笑)
修正しようとするとメンドクサイやつです。
-
- 変更の発散
- SRP(単一責任原則)違反。いろいろやり過ぎている。仕事が多すぎるので分担しましょう。
-
- 変更の分散
- 一つのことをいろんなクラスに分担させすぎている。責任もう少し集中させましょう。
-
- パラレル継承
- 「変更の分散」の特殊ケース。OCP(開放/閉鎖原則)違反。親クラスとサブクラスで責任を適切に分担しましょう。
加齢臭系
若かった頃（システムが小規模だった頃 or 開発の途中までは）はたぶん臭くなかったはず
メンドクサイ系の親戚筋
-
- 属性、操作の横恋慕
- クラスの役割分担が不明確になっている。役割分担を適切にしましょう
-
- メッセージの連鎖
- あるオブジェクトから取得したこのオブジェクトの知っているそのオブジェクトの...。全体を整理してみましょう。
-
- 不適切な関係
- クラス間が密結合してしまっている。自分のことは自分でしよう。
-
- 相続拒否
- 継承したはいいがオーバライドして変えまくって、結果別モノに...。LSP（リスコフの置換原則違反）。継承をやめるか、親の機能を子に移動するとか検討しましょう。
生臭い系
ほっとくと臭くなる類のものを放置系。
データモデリング不足、カプセル化の検討不足。もうちょっと手間かけて調理しましょう。
-
- データの群れ
- いつも同じセットで使用するデータ。オブジェクトに閉じましょう。
-
- 基本データ型への執着
- ValueObjectを作ってみましょう
-
- 一時的属性
- 変数(フィールド)はあるが使われたり使われなかったり。外部化またはNullObject化しましょう
汗臭い系
頑張った結果なのだが、汗臭くなっただけだった...。
-
- 疑わしき一般化
- 将来を見越しすぎて必要以上に「イイ感じにしよう」とがんばったが無駄だった。YAGNI原則(それは必要にならない)違反。「本当に必要になってから」実装することにして、それまでは「シンプルさ」を追求しましょう。
ウソ臭い系
ウソを付くつもりはなかったのに、結果的には嘘つき。
-
- 未熟なクラスライブラリ
- そのままでは使えないライブラリ。使えるように修正するか、ライブラリで全部やるのは諦めましょう。
-
- コメント
- コメントが古い、もしくはウソ。極力「コメントなし」でも読めるようにしましょう。コードでは表せないこと(なぜこういうロジックにしたか、など)だけをコメントにすることを目標に、コメントに頼らずまずはコードを改善しましょう。
テストがあるか？なければ書く - PhpUnit
リファクタリングは、「“プログラムの動作を変えずに”内部構造を改善すること』です。
プログラムの動作が変わっていないことを確認するには、テストするしかありません。
テストしましょう。
テストを書くと気づきますが、
**「テストしやすいコードを書く」**ことは非常に重要です。
リファクタリングする
「コードの匂い」毎にテクニックはありますが、全部は説明しきれないので、今回のワークショップでは簡単なものを取り上げます。
前回作成したアップロードの関数が「長すぎるメソッド」の匂いがしますので、「メソッドの抽出」をおこなってリファクタリングしてみます。
テストする
テストして結果を確認しましょう。パスすればリファクタリング完了です。
うまくテストコードを書くためには、
何をテストして何をテストしないか*をきちんと分けることが重要です。
テストしない部分は「モック」として実装し、テストを効率的に進めていきます。
1に戻る
リファクタリングは常に実施する必要があります。
技術的負債は計画的、継続的に減らしていきしましょう。
ワークショップメニュー
- 事前準備
- ブランチ整備
- composer設定
- phpUnitインストール
- テスト用DBの作成
- Lesson1 PHPUnitの設定と簡単なテスト作成
- Lesson2 アップロード機能のテスト
- Lesson3 アップロード機能のリファクタリング
という感じですすめます。
事前準備
事前準備は毎回同じなので、別エントリにまとめています。
全12回の勉強会でやっているGitの使い方 - AWS上で構築するRESTfulアプリ勉強会～Web開発ワークショップ～ - Qiitaを参照してください。
やることは、
- gitのブランチを整えて今回用ブランチ
vol/10を作成する
です。まずこれをやりましょう。
それと、第5回と第6回に不参加の方は、テーブルの修正が必要です。
- 第5回
- ユーザ登録用のテーブル作成（ログイン機能実装のため）
- 第6回
- TODO一覧テーブルへの列追加(owner列とassignee列。担当者アサイン機能実装のため）
をやってますので、それぞれ下記リンク先を参照して実施して下さい。
ユーザ登録用のテーブル作成
TODO一覧テーブルへの列追加
あと今回は、テストコード作成にphpUnitを使用します。composerを使用してインストールしますので、composerの準備もしましょう。
composer設定、phpUnitインストール
Composerは、phpで使用するパッケージ管理システムです。今回はこのComposerを使用してphpUnitをインストールします。
Composerのダウンロード
まずはComposerのダウンロードです。
/var/www/study/rest-studディレクトリに移動してwgetコマンドでダウンロードしましょう。
cd /var/www/study/rest-study
wget http://getcomposer.org/composer.phar
composer.jsonを修正
次に、composer.json(Composerで管理するモノを定義するファイル)を修正します。
debug_kitはとりあえず不要なので削除しています。
今回はComposer自体やcomposer.jsonのフォーマットについてなどは深入りしません。あしからず...
{
"name": "cakephp/cakephp",
"description": "The CakePHP framework",
"type": "library",
"keywords": ["framework"],
"homepage": "http://cakephp.org",
"license": "MIT",
"authors": [
{
"name": "CakePHP Community",
"homepage": "https://github.com/cakephp/cakephp/graphs/contributors"
}
],
"support": {
"issues": "https://github.com/cakephp/cakephp/issues",
"forum": "http://stackoverflow.com/tags/cakephp",
"irc": "irc://irc.freenode.org/cakephp",
"source": "https://github.com/cakephp/cakephp"
},
"require": {
"php": ">=5.2.8",
"ext-mcrypt": "*"
},
"require-dev": {
- "phpunit/phpunit": "3.7.*",
- "cakephp/debug_kit" : "2.2.*"
+ "phpunit/phpunit": "3.7.*"
},
"bin": [
"lib/Cake/Console/cake"
]
}
Composerを実行してphpUnitをインストール
下記の通り実行します。
php composer.phar install
テスト用DBの作成
普段つかているのはstudyデータベースですが、これをそのままコピーしてstudy_testデータベースを作成しましょう。
phpMyAdminを使用します。
まず、phpMyAdminにログインしましょう。
URLは、http://(PublicIP)/phpMyAdmin/です。
変更していなければ、ユーザ名study、パスワードstudyでログインできます。
その後下記手順で、studyデータベースをコピーしてstudy_testデータベースを作成します。
- ログイン後、
studyデータベースを選択した状態で、下図の通り「操作」をクリック。
-
次に「データベースのコピー先」で、名前を「
study_test」に設定、「構造のみ」を選択し、実行します。
-
下図の通り、完了です。
準備ができたら、Lesson1です！
Lesson1 phpUnitの設定と簡単なテスト作成
Lesson3で、前回作成したアップロード機能のリファクタリングを行いますが、まずはもっと簡単な機能でphpUnitでのテストコード作成を試してみましょう。
- todoの一覧取得(
TodolistsController::index()) - todoの１件取得(
TodoListsController::view())
の2つのメソッドのテストコードを書いてみます。
それぞれ、下記のJsonで表されるデータを登録し、それが正しく取得できることを確認します。
TodolistsController::index()
[
{
"TodoList": {
"id": "1000",
"todo": "牛乳を買う",
"status": "1",
"owned": true,
"assigned": true
},
"Owner": {
"id": "1000",
"name": "山田太郎""
},
"Assignee": {
"id": "1000",
"name": "山田太郎"
}
}
]
TodolistsController:: view()
{
"TodoList": {
"id": "1000",
"todo": "牛乳を買う",
"status": "1",
},
"Owner": {
"id": "1000",
"name": "山田太郎"
},
"Assignee": {
"id": "1000",
"name": "山田太郎"
}
}
編集するファイル一覧
| 編集 | file | 編集概要 |
|---|---|---|
| 追加 | app/Config/core.php | phpUnit用の設定を追加 |
| 修正 | app/Config/database.php | テスト用データベース設定の追加 |
| 追加 | app/Test/Fixture/TodoListFixture.php | todoテストデータ登録処理 |
| 追加 | app/Test/Fixture/UserFixture.php | userテストデータ登録処理 |
| 追加 | app/Test/Case/Controller/TodoListsControllerTest.php | メインのテストコード |
app/Config/core.php
phpUnitの各クラスが自動でロードされるよう設定を追加します。
〜略〜
/**
* Configure the cache for model and datasource caches. This cache configuration
* is used to store schema descriptions, and table listings in connections.
*/
Cache::config('_cake_model_', array(
'engine' => $engine,
'prefix' => $prefix . 'cake_model_',
'path' => CACHE . 'models' . DS,
'serialize' => ($engine === 'File'),
'duration' => $duration
));
+
+require_once ROOT . DS . 'vendor' . DS . 'autoload.php';
-
autoload.php内に、Composerでインストールしたクラスを自動で読み込む設定が入っていますので、Pathを指定して読み込んでいます。
app/Config/database.php
作成したテスト用データベースへの接続情報を設定します。
〜略〜
class DATABASE_CONFIG {
public $default = array(
'datasource' => 'Database/Mysql',
'persistent' => false,
'host' => 'localhost',
'login' => 'study',
'password' => 'study',
'database' => 'study',
'prefix' => '',
'encoding' => 'utf8',
);
public $test = array(
'datasource' => 'Database/Mysql',
'persistent' => false,
'host' => 'localhost',
- 'login' => 'user',
- 'password' => 'password',
- 'database' => 'test_database_name',
+ 'login' => 'study',
+ 'password' => 'study',
+ 'database' => 'study_test',
'prefix' => '',
- //'encoding' => 'utf8',
+ 'encoding' => 'utf8',
);
}
app/Test/Fixture/TodoListFixture.php
テスト用データベースにTodoデータを登録する「Fixture(フィクスチャ)」クラスです。
<?php
class TodoListFixture extends CakeTestFixture {
public $import = 'TodoList';
public $records = array (
array (
"id" => 1000,
"todo" => "牛乳を買う",
"status" => "1",
"owner" => 1000,
"assignee" => 1000
)
);
}
-
public $import = 'TodoList';で、database.phpの$defaultで記載されたデータベース(study)のTodoListテーブルの構造がコピーされます。 -
public $records = array ( ... )で、レコードが作成されます。
app/Test/Fixture/UserFixture.php
テスト用データベースにUserデータを登録する「Fixture(フィクスチャ)」クラスです。
<?php
App::uses('BlowfishPasswordHasher', 'Controller/Component/Auth');
class UserFixture extends CakeTestFixture {
public $import = 'User';
public $records;
public function init() {
$this->records = array (
array (
"id" => 1000,
"username" => "yamada",
"name" => "山田太郎",
"password" => (new BlowfishPasswordHasher())->hash("yamada")
)
);
parent::init();
}
}
TodoListFixture.phpと同様、$importを$recordsを設定しますが、パスワードをハッシュ化する処理が必要なため、init関数内で$recordsを設定する処理を実装しています。
app/Test/Case/Controller/TodoListsControllerTest.php
メインのテストコードです。
<?php
App::uses('AppController', 'Controller');
class TodoListsControllerTest extends ControllerTestCase {
public $fixtures = array (
'app.todo_list',
'app.user'
);
/**
* 準備
* @return Controller
*/
public function setUp() {
parent::setUp();
$mocks = array (
'components' => array (
'Auth' => array (
'user'
)
)
);
//TodoListsControllerを生成
$controller = $this->generate('TodoLists', $mocks);
//Authコンポーネントのuserメソッドをスタブにする
$loginUser = array (
"id" => "1000",
"username" => "yamada",
"name" => "yamada"
);
$controller->Auth->staticExpects($this->any())
->method('user')
->will($this->returnValue($loginUser));
$this->controller = $controller;
}
/**
* index関数のテスト
*/
public function testIndex() {
$this->testAction('/todo_lists.json', array (
'method' => 'get'
));
$result = $this->vars['res'];
$expected = array (
array (
"TodoList" => array (
"id" => "1000",
"todo" => "牛乳を買う",
"status" => "1",
"owned" => true,
"assigned" => true
),
"Owner" => array (
"id" => "1000",
"name" => "山田太郎"
),
"Assignee" => array (
"id" => "1000",
"name" => "山田太郎"
)
)
);
$this->assertEquals($expected, $result);
}
/**
* view関数のテスト
*/
public function testView() {
$this->testAction('/todo_lists/1000.json', array (
'method' => 'get'
));
$result = $this->vars['res'];
$expected = array (
"TodoList" => array (
"id" => "1000",
"todo" => "牛乳を買う",
"status" => "1"
),
"Owner" => array (
"id" => "1000",
"name" => "山田太郎"
),
"Assignee" => array (
"id" => "1000",
"name" => "山田太郎"
)
);
$this->assertEquals($expected, $result);
}
}
ControllerTestCaseクラスを継承した、TodoListsControllerをテストする為のクラスです。大きく、下記に分かれますので順番に説明します。
-
$fixtures変数- フィクスチャを設定します。これで、テスト実行時にデータが登録されます。
-
setUp関数- テスト対象となる
TodoListsControllerを、一部処理をモックにした状態で生成します。-
generate関数を実行します。作成するControllerの名前(TodoLists)と、モックに関する情報を渡します。- ここでは、
Authコンポーネントのuser関数(ログイン中ユーザの情報を返す関数)をモックにし、$loginUserで設定した情報を返す様にスタブ関数にしています。これでログイン状態をシミュレートしています。
- ここでは、
-
- テスト対象となる
- index関数のテストコード
-
testActionでurlとmethodを指定することで、クライアントからのアクセスをシミュレートできます。 - その戻り値のJsonが、フィクスチャで１件だけ登録したTODOデータと一致することを
assertEquals関数を使用して確認しています。
-
- view関数のテストコード
- 同じく、
testAction関数でテストを実行しています。
- 同じく、
テスト実行
さて、テストコードができました！実行方法を説明します！
ブラウザから、またはコマンドラインからでも実行できます。
ブラウザで実行
ブラウザで次のURLを表示してみましょう。
http://(PublicIP)/rest-study/test.php
下記の画面が出ます！作成したテストクラスが表示されているのでクリックしましょう。
テストが実行されました。下図のように緑のバーが表示されていたら成功です。
試しに、フィクスチャで設定するデータを変えてみました。
フィクスチャに登録するTodoを、「牛乳を買う」から「犬の散歩をする」に修正して実行すると、
コマンドラインから実行
/var/www/study/rest-study/app/Consoleに移動し、./cake test app Controller/TodoListsControllerと実行します。
cd /var/www/study/rest-study/app/Console
./cake test app Controller/TodoListsController
- テスト成功時
- テスト失敗時
実装
では、実際に修正してテストを実行してみましょう。
-
app/Config/core.phpを上記の通り作成。 -
app/Config/database.phpを上記の通り修正。 -
app/Test/Fixture/TodoListFixture.phpを上記の通り作成。 -
app/Test/Fixture/UserFixture.phpを上記の通り作成。 -
app/Test/Case/Controller/TodoListsControllerTest.phpを上記の通り作成。 -
テストを実行!
-
Gitにコミット
GitHubでのdiff表示へのリンク
第10回 Lesson1 phpUnitの設定と簡単なテスト作成 · suzukishouten-study/rest-study@a17ff3a
Lesson2へ!
Lesson2 アップロード機能のテスト
Lesson2では、今回リファクタリングするアップロード機能のテストを先に作っておきます。
実は、前回作成したアップロードのプログラムにバグがありました！直しています。
編集するファイル一覧
| 編集 | file | 編集概要 |
|---|---|---|
| 修正 | app/Test/Case/Controller/TodoListsControllerTest.php | アップロード処理のテスト追加 |
| 修正 | app/Model/TodoList.php | テスト用DBを見に行く為の修正 |
| 修正 | app/Controller/TodoListsController.php | バグフィックスと一部スタブ化するための修正 |
app/Test/Case/Controller/TodoListsControllerTest.php
アップロード処理のテスト関数を追加します。
〜略〜
public function setUp() {
parent::setUp();
$mocks = array (
+ 'methods' => array (
+ 'getUploadFileParams'
+ ),
'components' => array (
'Auth' => array (
'user'
@@ -90,4 +93,88 @@ public function testView() {
$this->assertEquals($expected, $result);
}
+ /**
+ * upload関数のテスト
+ * アップロードファイル1つ、2件を正常登録
+ */
+ public function testUploadOKFile() {
+ // 一時保存されるアップロードファイル
+ $postFileName = 'testUploadOKFile.txt';
+ $tmpFileName = tempnam('/tmp', $postFileName);
+ // アップロードされてきた体でファイルを作成しておく
+ file_put_contents($tmpFileName, array (
+ "ほげ\n",
+ "12345\n"
+ ));
+ // POSTされるフォームデータ
+ $uploadFormData = array (
+ array (
+ 'name' => $postFileName,
+ 'tmp_name' => $tmpFileName
+ )
+ );
+ // フォームデータ取得関数は、上で用意したフォームデータを返すようにスタブにする
+ $this->controller->expects($this->any())
+ ->method('getUploadFileParams')
+ ->will($this->returnValue($uploadFormData));
+ // テスト実行
+ $result = $this->testAction('/todo_lists/upload.json', array (
+ 'method' => 'post'
+ ));
+ // 結果取得 / 確認
+ $result = $this->vars['response'];
+ $expected = '2件のTODOを登録しました。';
+ $this->assertEquals($expected, $result);
+ }
+
+ /**
+ * upload関数のテスト
+ * アップロードファイル2つ、3件を正常登録, 1件はバリデーションエラー
+ */
+ public function testUploadOKandNGFile() {
+ //一時保存されるアップロードファイル1
+ $postFileName1 = 'testUploadOKandNGFile1.txt';
+ $tmpFileName1 = tempnam('/tmp', $postFileName1);
+ //アップロードされてきた体でファイルを作成しておく
+ file_put_contents($tmpFileName1, array (
+ "ほげ\n",
+ "12345\n",
+ "12345\n"
+ ));
+ //一時保存されるアップロードファイル2
+ $postFileName2 = 'testUploadOKandNGFile2.txt';
+ $tmpFileName2 = tempnam('/tmp', $postFileName2);
+ //アップロードされてきた体でファイルを作成しておく
+ file_put_contents($tmpFileName2, array (
+ "ふが\n",
+ "12345\n" //これは重複でエラーになる
+ ));
+ // POSTされるフォームデータ
+ $uploadFormData = array (
+ array (
+ 'name' => $postFileName1,
+ 'tmp_name' => $tmpFileName1
+ ),
+ array (
+ 'name' => $postFileName2,
+ 'tmp_name' => $tmpFileName2
+ )
+ );
+
+ //TodoListControllerを生成
+ //フォームデータ取得関数は、上で用意したフォームデータを返すようにスタブする
+ $this->controller->expects($this->any())
+ ->method('getUploadFileParams')
+ ->will($this->returnValue($uploadFormData));
+ //テスト実行
+ $result = $this->testAction('/todo_lists/upload.json', array (
+ 'method' => 'post'
+ ));
+ //結果取得 / 確認
+ $result = $this->vars['response'];
+ $this->assertEquals('3件のTODOを登録しました。', $result['errors'][0][0]);
+ $this->assertEquals('以下のエラーが発生しました。', $result['errors'][1][0]);
+ $this->assertEquals('file:testUploadOKandNGFile1.txt - line: 3: 同じ内容のTODOが既に登録されています。', $result['errors'][2][0]);
+ $this->assertEquals('file:testUploadOKandNGFile2.txt - line: 2: 同じ内容のTODOが既に登録されています。', $result['errors'][3][0]);
+ }
-
setUp関数-
TodoListControllerのgetUploadFileParams関数をモックにする設定を追加しています。
-
-
testUploadOKFile関数- アップロード処理のテストその1です。ファイルを1個だけアップロードし、ファイル内に記載されている2行のTODOが正しくDBに登録されることを確認します。
-
getUploadFileParams関数をテストコード内で指定した情報を返すようにスタブ化しています。-
getUploadFileParamsは前回は実装しておらず、upload関数内でクライアントからpostされたフォーム情報を取得するため、$files = $this->request->params['form'];と直接書いてました。スタブ化するために今回関数として$files = $this->request->params['form'];とするだけの処理を切り出しています。
-
- あとは、Lesson1の
testIndex関数とtestView関数と同様に、testAction関数でテストを実行し、assertEquals関数で結果を確認しています。 -testUploadOKandNGFile関数 - アップロード処理のテストその2です。ファイルを2個アップロードし、合計5件のTODOが登録対象となりますが、3件は正しくDBに登録され、2件はバリデーションエラー(同じ内容のTODOは登録できない、というルールに引っかかる)となることを確認します。
- スタブの作成方法は上の
testUploadOKFile関数と同様です。 - テストの実行方法、結果確認方法も同様です。
app/Model/TodoList.php
テスト用DBを見に行く為の修正をしています。
〜略〜
//独自バリデーションルール
//担当者として指定されたIDがusersテーブルに存在するかチェックする
public function existsUser($userId){
- $userModel = new User();
+ $userModel = ClassRegistry::init('User');
$count = $userModel->find('count', array('conditions'=>array('id'=>$userId), 'recursive' => -1));
return $count > 0;
}
〜以下略〜
独自バリデーション用の関数として、担当者として指定されたユーザのIDの存在チェックをしているexistsUser関数がありますが、Userモデルを生成するのに、
new User()
としているのを、
ClassRegistry::init('User')
に変更しています。これは、new User()としてしまうとテスト実行時にdatabase.phpのtest変数で設定した接続情報でなく、defaultの設定を見に行ってしまうためこのようにしています。
Modelの生成ははじめからこの方式でやっといたほうがいいですね！
app/Controller/TodoListsController.php
先程述べた、クライアントからpostされたフォーム情報を取得する処理のスタブ化とバグフィックスを行っています。
〜略〜
}
public function upload() {
- $files = $this->request->params['form'];
+ $files = $this->getUploadFileParams();
$owner = $this->Auth->user()['id'];
$numTodos = 0;
+ $errors = array ();
foreach ( $files as $file ) {
$fileName = $file['name'];
$filePath = $file['tmp_name'];
$todos = file($filePath, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
$assignee = $owner;
- $errors = array ();
$lineNo = 1;
foreach ( $todos as $todo ) {
$data = array ();
$data['todo'] = $todo;
$data['status'] = 0;
$data['owner'] = $owner;
$data['assignee'] = $assignee;
$res = $this->TodoList->save($data);
if ($res) {
$numTodos++;
} else {
if (count($this->TodoList->validationErrors) > 0) {
foreach ( $this->TodoList->validationErrors as $validationErrorsOfLine ) {
$title = 'file:' . $fileName . ' - line: ' . $lineNo . ': ';
foreach ( $validationErrorsOfLine as $validationError ) {
$errors[] = array (
$title . $validationError
);
}
}
}
}
$this->TodoList->create();
$lineNo++;
}
}
if (count($errors) > 0) {
$this->TodoList->validationErrors = $errors;
$response = $this->editResponse(false);
array_unshift($response['errors'], array (
'以下のエラーが発生しました。'
));
if ($numTodos > 0) {
array_unshift($response['errors'], array (
$numTodos . '件のTODOを登録しました。'
));
}
} else {
$response = $numTodos . '件のTODOを登録しました。';
}
$this->set(compact('response'));
$this->set('_serialize', 'response');
}
+ protected function getUploadFileParams(){
+ return $this->request->params['form'];
+ }
+
//レスポンスを編集
private function editResponse($res){
if($res){
〜以下略〜
- スタブ化
-
getUploadFileParams関数を追加し、クライアントからpostされたフォーム情報を取得する処理($this->request->params['form'])をupload関数から追い出しています。
-
- バグフィックス
- バリデーションエラー情報を格納する
$errors変数を初期化している$errors = array ()の場所がおかしかったので位置を変えています。修正前の状態だと、1個目のファイルのバリデーションエラー情報がクリアされてしまいますね
- バリデーションエラー情報を格納する
実装
では、実際に修正してテストてみましょう。
-
app/Test/Case/Controller/TodoListsControllerTest.phpを上記の通り修正。 -
app/Model/TodoList.phpを上記の通り修正。 -
app/Controller/TodoListsController.phpの、まずスタブ化の部分だけ実装。 -
テスト実行。下記のようにエラーが検知されるはずです。
バグのためテスト失敗
続いて、バグフィックス後再テストします。
-
app/Controller/TodoListsController.phpのバグフィックス部分を実装。 -
テスト実行。今度は成功するはずです
テスト成功
-
Gitにコミット
GitHubでのdiff表示へのリンク
第10回 Lesson2 アップロード機能のテスト · suzukishouten-study/rest-study@fa3f13f
では、いよいよLesson3でリファクタリングを行います。
Lesson3 アップロード機能のリファクタリング
修正するのはapp/Controller/TodoListsController.phpのみです。
まず、修正前のアップロード関数を見てみましょう。
public function upload() {
$files = $this->getUploadFileParams();
$owner = $this->Auth->user()['id'];
$numTodos = 0;
$errors = array ();
foreach ( $files as $file ) {
$fileName = $file['name'];
$filePath = $file['tmp_name'];
$todos = file($filePath, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
$assignee = $owner;
$lineNo = 1;
foreach ( $todos as $todo ) {
$data = array ();
$data['todo'] = $todo;
$data['status'] = 0;
$data['owner'] = $owner;
$data['assignee'] = $assignee;
$res = $this->TodoList->save($data);
if ($res) {
$numTodos++;
} else {
if (count($this->TodoList->validationErrors) > 0) {
foreach ( $this->TodoList->validationErrors as $validationErrorsOfLine ) {
$title = 'file:' . $fileName . ' - line: ' . $lineNo . ': ';
foreach ( $validationErrorsOfLine as $validationError ) {
$errors[] = array (
$title . $validationError
);
}
}
}
}
$this->TodoList->create();
$lineNo++;
}
}
if (count($errors) > 0) {
$this->TodoList->validationErrors = $errors;
$response = $this->editResponse(false);
array_unshift($response['errors'], array (
'以下のエラーが発生しました。'
));
if ($numTodos > 0) {
array_unshift($response['errors'], array (
$numTodos . '件のTODOを登録しました。'
));
}
} else {
$response = $numTodos . '件のTODOを登録しました。';
}
$this->set(compact('response'));
$this->set('_serialize', 'response');
}
ちょっと長いですね。これくらいならまだマシな方ではありますが、長くてロジックがわかりにくくなっています。
冒頭で紹介した不吉な匂いの、「長すぎるメソッド」の臭いがします。
ここはリファクタリングです！
ロジックを追ってみると下記の部分にわかれていることがわかります。
- クライアントがアップロードを実行した際のPOSTデータを取得する
- ログイン中ユーザのIDを取得する
- アップロードされたファイル群を読み込んでTODOとしてDBに登録する
- アップロードされたファイルを1つを読み込んで配列に格納する
- 配列に格納されたTODOをひとつずつDBに登録する
- バリデーションエラーがあった場合、内容を整形する
- アップロード処理結果のメッセージをクライアント向けに整形する
これらを関数に分割してみましょう。
リファクタリングの中でも最も初歩的な「メソッドの抽出」という技(技と言う程でもないですが...)です！
修正後のアップロード関数と「メソッドの抽出」により抽出した関数は下記のとおりです。
public function upload() {
$fileUploadParams = $this->getUploadFileParams();
$loginUserId = $this->getLoginUserId();
$owner = $loginUserId;
$assignee = $loginUserId;
$errors = array();
$numRegists = $this->registerFilesAsTodos($fileUploadParams, $owner, $assignee, $errors);
$response = $this->editUploadResponse($numRegists, $errors);
$this->set(compact('response'));
$this->set('_serialize', 'response');
}
//アップロードのPOSTデータを取得する
protected function getUploadFileParams(){
return $this->request->params['form'];
}
//ログイン中ユーザのIDを取得する
protected function getLoginUserId(){
return $this->Auth->user()['id'];
}
//アップロードされたファイル群を読み込んでTODOとしてDBに登録する
private function registerFilesAsTodos($fileUploadParams, $owner, $assignee, &$errors){
$numRegists = 0;
//$errors = array();
foreach ( $fileUploadParams as $fileUploadParam ) {
$fileName = $fileUploadParam['name'];
$filePath = $fileUploadParam['tmp_name'];
$todos = $this->readUploadTodoFile($filePath);
$numRegists += $this->registerTodos($fileName, $todos, $owner, $assignee, $errors);
}
return $numRegists;
}
// アップロードされたファイルを読み込んで配列に格納して返す
protected function readUploadTodoFile($filePath){
return file($filePath, FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
}
// 配列に格納されたTODOをDBに登録する
private function registerTodos($fileName, $todos, $owner, $assignee, &$errors){
$numRegists = 0;
$lineNo = 1;
foreach ( $todos as $todo ) {
$record = array ();
$record['todo'] = $todo;
$record['status'] = 0;
$record['owner'] = $owner;
$record['assignee'] = $assignee;
$res = $this->TodoList->save($record);
if ($res) {
$numRegists++;
} else {
$validationErrors = $this->TodoList->validationErrors;
if (count($validationErrors) > 0) {
$this->formatValidationErrorMessage($fileName, $lineNo, $validationErrors, $errors);
}
}
$this->TodoList->create();
$lineNo++;
}
return $numRegists;
}
// バリデーションエラーの内容を整形する
private function formatValidationErrorMessage($fileName, $lineNo, $validationErrors, &$errors){
foreach ( $validationErrors as $validationErrorsOfLine ) {
$title = 'file:' . $fileName . ' - line: ' . $lineNo . ': ';
foreach ( $validationErrorsOfLine as $validationError ) {
$errors[] = array (
$title . $validationError
);
}
}
}
// アップロード処理結果のメッセージをクライアント向けに整形する
private function editUploadResponse($numRegists, $errors){
if (count($errors) > 0) {
$this->TodoList->validationErrors = $errors;
$response = $this->editResponse(false);
array_unshift($response['errors'], array (
'以下のエラーが発生しました。'
));
if ($numRegists > 0) {
array_unshift($response['errors'], array (
$numRegists . '件のTODOを登録しました。'
));
}
} else {
$response = $numRegists . '件のTODOを登録しました。';
}
return $response;
}
上で説明したロジックを、下表の通り「メソッドの抽出」を行っています。
処理 | 抽出したメソッド
-----------------------------------------------------+----------------------------
クライアントがアップロードを実行した際のPOSTデータを取得する | getUploadFileParams
ログイン中ユーザのIDを取得する | getLoginUserId
アップロードされたファイル群を読み込んでTODOとしてDBに登録する | registerFilesAsTodos
アップロードされたファイルを1つを読み込んで配列に格納する | readUploadTodoFile
配列に格納されたTODOをひとつずつDBに登録する | registerTodos
バリデーションエラーがあった場合、内容を整形する | formatValidationErrorMessage
アップロード処理結果のメッセージをクライアント向けに整形する | editUploadResponse
それぞれの処理は、単に抽出しただけなので修正前と同じです。
情報をやり取りする為引数と戻り値で受け渡ししています。
バリデーションエラー情報を、&$errorsとして参照渡ししています。
参照渡しはどちらかと言うとアンチパターンですし、ちょっと引数が多いので「多すぎる引数」の臭いが若干します。
「引数オブジェクトの導入」というリファクタリングを行うべきかもしれませんが、今回はご愛嬌ということで
実装
では、実際に修正してテストを実行してみましょう。
-
app/Controller/TodoListsController.phpを上記の通り修正。 -
テスト実行。成功すれば下記の様になります！
テスト成功
-
Gitにコミット
GitHubでのdiff表示へのリンク
第10回 Lesson3 アップロード機能のリファクタリング · suzukishouten-study/rest-study@e774e5e
以上です！
次回予告
次回は「リファクタリング（クライアント編）」です。
今回やったことと同じようなことをクライアント側でもやってみたいと思います。
コメント/フィードバックお待ちしております。
参加者の方も、そうでない方もお気づきの点があればお願い致します。