皆さんこんにちは
今年も弊社でアドカレやるっていうので、参加してみました。
ここ一年、自社サービスを開発・運用していく上でベストかどうかはわからないけれど、割とベターなテスト及びコードを書く上での指針が確立して来ました。
私はプログラミングについて理論などを正式に学習したことはないので、一部では邪道と呼ばれることをやっているかもしれませんが、まあ、サービスの開発・運用で困らなければいいのではないかと。
それに、世間ではもてはやされていたやり方が、全然肌に合わなかったりしたので、大事なのはエンジニアにとっていかにストレスなく作業できるかなのではないかなって思っています。
TL;DR
- テストは開発をサポートするもので、邪魔するものであってはならない
- テストの単位はリクエストごとでいい
- クラスごとのテストはごく一部のライブラリとかだけでやる
- ビジネスロジックはコントローラに書いていい、というか MVC にこだわらない
- ビジネスロジックの参照階層はなるべく浅く
- 共通部分は積極的にリファクタリング
- テストファーストではない、とてもゆるい感じではあるが、TDDのようなものに落ち着いた
アタリマエのこともあれば、不思議なこともあるでしょうが、こんな感じです。
開発・運用を補助するテスト方針
サービスの成長において、テストは欠かせません。
しかし、テストを書くことで開発が滞るというのも問題です。
テストを書く行為は、開発の手助けになるべきで、開発を阻害するものになってはいけません。
テストは何のために存在するのか
耳にタコかもしれませんが、先にテストを入れる目的をおさらいしておきましょう。
テストコードはあくまで「現在の仕様に従って動いていることを保証する」ためのものですので、テストがなくてもアプリは動きます。
しかし、継続的に成長するサービス、つまりアプリがアップデート及びバージョンアップしていくとき、現在の動作を保証しているというのはとても強い武器となります。
テストが通っていることによって、以下の効果を得ることができます。
- それまでの仕様が壊れていないことを確かめつつ、新しい仕様を追加していける
- 効率の悪いコード、見通しの悪いコードが存在するとき、現行の仕様を維持しつつ、改善ができる ( リファクタリング )
一つ目は、サービスを拡張するためにどんどん仕様を追加していくときに、前回仕様が通っているかを確かめる手間を大幅に省くことができます。
二つ目は、コードの見通しを良くしていくことで、保守性を高めることにつながります。
再度意識しておくべきことがあります。
テストがなくてもコードは動きます。
つまり、テストは開発をサポートするための手法ではありますが、これがメインではない、ということです。
どうやってテストするのか
PHPはWebアプリに特化した言語なので、当然構築するシステムはWebアプリになるわけですが、Webアプリはとても単純な構造をしています。
つまり、リクエストを受け取ったらレスポンスを返す、というものです。
であれば、テストの方針はとても簡単です。
「リクエストに対するレスポンスと、それに伴うデータの変更をテストする」としておけば、問題ないでしょう。
例えばこんな感じです
// リクエストの挙動についてのテスト ( ここではPOSTのテスト )
$this->post('/items', ['name'=> 'お団子', 'category' => 1])
->seeJson(['items' => [['name' => 'お団子', 'category' => 1]]);
// リクエストに伴うデータの追加をテスト
$sample = Item::where('name', 'お団子')->first();
$this->assertNotNull($sample);
$this->assertEquals(1, $sample->category);
最近のLaravelやCakePHPではリクエストのテストがサポートされていますし、Codeceptionのようにより広範でリクエストのテストができるようになりましたので、このようなテストがよりやりやすくなっているといえます。
また、テストの際のDBなどは、コンテナでテスト用のものを用意して、なるべくモックしないようにします。より実際の動きを再現するためですし、イマドキはコンテナ使えばモックするよりも実動作を使ったほうが断然楽です。
一方で、クラスごとのテストは本当に限定的な状況でしか行いません。というか、リクエストのテストの時点で大体のクラスはほぼテストしちゃっているので、いらないと言ったほうが正しいかも。
僅かに書いたテストは、例えば以下のように「zipを解凍して各エントリーが正しく取り出せている」みたいな、動作が他に依存しないものについては先にテストで動作確認してから、ビジネスロジックに組み込んでます。
public function testZip()
{
$file = new File(base_path('something.zip'), 'something.zip', '', 100, null, true);
$zip = new MyZip($file);
$i = 0;
foreach ($zip->load() as $key => $fp) {
$i++;
$str = stream_get_contents($fp, -1, 0);
if ($key == 'test1.txt') {
$this->assertEquals("こんにちは", $str);
} else {
throw new \Exception("余計なものが出てきた: $key");
}
}
$this->assertEquals(1, $i, 'ファイル数は1であるべき');
}
テストケースをむやみに探さない
テストケースが多いほうが、確かめがたくさんできていいように思いますが、やたらめったらテストケースを探しまくるのも問題です。テストケースを探すのに手間取って本来の開発が遅延してしまっては元も子もありません。
典型的な例として、バリデーションのテストがあります。
3このフィールドに対して2個ずつバリデーションルールがあったとすると、バリデーションの組み合わせは$3^3 = 27$個発生します( エラーなしを含む )。
この組み合わせをすべてやると、それだけで27このテストが発生するわけですが、これは流石に書くのも億劫になります。
やり方は個人差がありますが、自分のやり方は「とりあえず適当なエラーケースを作ってそれだけテストし、問題があったら後から追加する」というやり方にしています。
コードの書き方方針
テストコードをどうかけばいいかはだいたい決まったので、次に実コードを書くに当たって、どう書けばいいかを考えます。
ファットコントローラを容認する
私の中でここ1,2年で劇的に変わった考え方は、「ファットコントローラを容認する」ということです。
私も一昔前はファットコントローラ撲滅にハマっていましたが、前述したリクエストのテストによって、コントローラもテストされているので、コントローラが太っても特に気にならなくなりました。
そして、開発のスピードを高めるとき、最も手っ取り早いのは「全てのロジックを一箇所に書く」というやり方で、言い換えれば、あれこれ考える前に、とりあえずコントローラに直にスパゲティでいいので動くものを書けばいいということです。
参照階層が多いわかりにくいコード
私がファットコントローラを許容する前に書いていたのは、次のような書き方です。
まず、リクエストを受け取り、モデルに処理を渡して結果をビューに表示するコントローラがあります。
class A extends Base
{
public function getSomething(Request $request)
{
$model = new SomethingFacade();
$model->exec($request->all());
return $this->view($model);
}
}
この時点で、この処理が何をしたいのか、まるでわかりません。
具体的な処理を見るためにはSomethingFacadeの中身を見る必要があります。
class SomethingFacade extends FacadeBase
{
protected function main_process($data)
{
$this->some = new SomeProcess()->getSomething($data)
$this->other = new OtherProcess()->getOthers($data)
}
}
どういう処理をしているのか見に来たら、また別の処理に投げているし、そもそもexecはどこ行った状態になりました。
私の場合、ベースクラスにexecを置いていて
abstract class FacadeBase
{
public function exec($data)
{
$this->createHeader($data);
$this->main_process($data);
$this->createFooter($data);
}
private function createHeader($data){}
private function createFooter($data){}
abstract protected function main_process($data)
}
みたいな感じになっていました。
このような深い参照階層を作り込んだ結果、コードが追いにくい状態になっていました。
処理を作るごとにモデルがどんどん増えていくし、予め共通部分を予測してどのクラスを使うかまで考えてからでなければ実装にすら移れなくなっていて、開発が進むに連れて、開発スピードが遅くなるような状況になりました。
「あとで作り込む」というパラダイムシフト
結局、予め考え込んで作ろうとすると、モジュールが増えるに連れて、先に見ておかなければならない部分も増えてきたため、スピードが遅くなるというジレンマに陥ったわけです。
そこに、前述した広範囲をカバーするテストを知るにいたり、とっとと動くコードを作ってしまって、あとで共通項やら冗長な部分を修正してしまえばいいという考えに落ち着きました。
いわゆるパラダイムシフトのようなものです。
リファクタリングをする
というわけで、取りあえず動くコードを作ることにしたものの、そのまま放置しておくのは流石に精神衛生上良くないし、当然保守性も落ちるし、他のエンジニアから「きったねぇwww」と笑われてしまうのは目に見えています。
そこで、テストが通っているのをいいことに、内部をガンガン書き換えて、外に出すときはきれいな状態になっている、という状態を作ってしまおうと考えるわけです。
これがリファクタリングですね。
目的
リファクタリングの目的は、コードをキレイに整理して、保守性を高めることです。
リファクタリングのメインの作業は、共通処理を一つにまとめることです。他にも名前を変えたり無用なループを解消したりとありますが、メインとしてはこの作業となるでしょう。
共通部分をまとめる際に、私がよくやる手法は以下のとおりです。
- ミドルウェアや割り込み関数に前後処理をまとめる
- 共通処理を trait に追い出す
- 処理ライブラリを作成する
どれを選択するかは、共通項となった処理の内容によります。
ミドルウェアや割り込み関数を使う
処理の前後に発生する処理が共通している場合は、基本的にはミドルウェアや割り込み関数を使ってまとめておくのが良いと思っています。
Laravelのミドルウェアは実施順が決まっているので以下のように順を追って前後処理を設置することができます。
- ログ取得開始・処理時間計測開始
- ログインチェック
- 権限チェック
- メイン処理
- (処理なし)
- (処理なし)
- ログ取得終了・処理時間の記載
trait を使う
ミドルウェアに入れられないけど、処理の一部で共通項が出てきた場合はtraitを使うと見通しが良くなります。
例えば、次のような疑似コントロラーがあったとします。
class AController extends Controller
{
public function index()
{
$list = A::all();
return view('list', ['list' => $list]);
}
public function show($id)
{
$data = A::find($id);
return view('show', ['data' => $data]);
}
}
これと全く同じような構造のBControllerがあったとしましょう。
class BController extends Controller
{
public function index()
{
$list = B::all();
return view('list', ['list' => $list]);
}
public function show($id)
{
$data = B::find($id);
return view('show', ['data' => $data]);
}
}
この場合の共通化は以下のようにします。
trait Common
{
public function index()
{
$list = $this->model::all();
return view('list', ['list' => $list]);
}
public function show($id)
{
$data = $this->modell::find($id);
return view('show', ['data' => $data]);
}
}
class AController extends Controller
{
use Common;
protected $model = A::class;
}
class BController extends Controller
{
use Common;
protected $model = B::class;
}
このように、同じような機能であればtraitに追い出して、コントローラ側では設定をするだけというのはよくやるリファクタリングです。
Laravelのログイン処理のように、ログイン処理を幾つかのtraitに細分化して、必要な処理だけ使う、というのも可能です。
処理ライブラリを作る
普通にWebアプリを作っている限り、別途処理ライブラリを作る必要というのは殆どないと思いますし、これを作ると参照が多くなるので、追いにくくなるわけですが、通常の処理とは毛色の違う処理がある場合は、別の処理ライブラリを作るほうがわかりやすい場合が多いです。
例えば、ZIP化されたファイル群を展開して、ゴニョゴニョ処理をする場合、
public function something(Request $request)
{
$file = $request->file('upfile');
$zip = \ZipArchive();
$zip->open($file->getPathName());
for ($num = 0; $num < $zip->numFiles; $num++) {
$path = $zip->getNameIndex($num);
$path_arr = explode('/', $path);
$name = end($path_arr);
if (! empty($name)) {
$fp = $zip->getStream($path);
// 何かの処理
if (is_resource($fp)) {
fclose($fp);
}
}
}
}
とか書くくらいなら、
public function something(Request $request)
{
$file = $request->file('upfile');
$myzip = new MyZip($file);
foreach ($myzip->load() as $name => $fp) {
// 何かの処理
}
}
みたいなZipを別途処理してファイルエントリーを逐次提供してくれるライブラリを作ってしまったほうが、何がしたいのかがはっきりしているので、いいんじゃないかって思います。
結局どうなったのか
結局、全体の流れはこんな感じです。
- 取りあえず仕様どおり動くようにコードを書く
- 動いたコードにかぶせるようにリクエストのテストを書く
- 人に見せて恥ずかしそうなコードや共通化できる部分はリファクタリングする
とにかくストレスフリーで、何かにこだわる書き方も必要なく、自分のコードセンスに従って好きなように各スタイルですね。
何が大切だったのか
初めの方で述べたように、テストを入れることで、面倒なリグレッション確認をする必要がなくなり、開発・リファクタリングを躊躇なく進めることができるようになります。
しかし、あくまでテストは、そのような 「便利なもの」 以上のものにはなりません。
MVCも、Webアプリを記述する上で馴染みやすいアーキテクチャではありますが、別にこれに従わなければならないというものでもありません。
結局のところ、いちばん大切なのは「開発」すること、つまりは実コードを書いて動くものを提供することです。
テストを書くことが目的になってはいけないし、MVCに厳密に従うことをにこだわっても、開発を阻害するようであれば、良いスタイルとはいえなくなるでしょう。
まとめ
というわけで、開発を中心に据えてコードやテストを書きましょうという話でした。
昔は私もやれテストをクラスごとに書くんだ！とかモックを使わなければ！とかファットコントローラ死すべし、慈悲はない！などと喚いていたものですが、随分と考えが変わったなぁと思ったので、昔の自分を供養しつつ現在の考えを書きなぐってみました。
今回はこんなところです。