この記事はPARONYM Advent Calendar 2019の20日目の記事です。
こんにちはサーバーサイドエンジニアの波多野です！
パロニムに正式にjoinしてから約1年3ヶ月になりましたが、実は自分は業務委託の期間が半年間あったのでサーバーサイドの面倒を見初めてからは1年9ヶ月になります。
ちょうど1年9ヶ月前の今日(3月20日)に弊社プロダクトのTIGが正式にサービスインしました！
今回はサービスイン直後のサーバサイドが陥っていた状態に少しだけ触れようと思います。
サーバサイドのダークサイド化
動いてはいるが結構マズイ状態。
どんな状態かというと具体的には
・機能ごとに実装の仕方がバラバラ
・ファットコントローラな状態で何をやっているのかわからない
・DRYの原則を基本無視している
何が悪いのかひとつづつ見ていきましょう
機能ごとに実装の仕方がバラバラ
これは、決して実装の仕方が間違っている訳ではありませんが。
機能的には共通にできそうな処理があったとしても、記述やロジックなどがバラバラなため共通化が困難な状態になっていました。
おそらく背景として、サービスイン前は外注して短期間のうちに作り上げていただいたからかもしれません。
外注先で担当者ごとにプロジェクトのコーディング規約やレビューでコードの品質まで意識するリソースが割けなかったのでしょう。
共通化できない悪いところとしては
「同じコードを何度も書くことになる」
ということです。
これで済めばまだいい方です。
タスクとしては同じなのに、実装が異なると修正に2倍のコストがかかります。
そのため最初に、「同じような処理を同じ記述にする」リファクタをしました。
ファットコントローラな状態で何をやっているのかわからない
弊社のサーバサイドはCakePHP3で実装されています。CakePHPを使っている人でしたら一度は出くわしたことがあると思いますが、ビジネスロジックや細かい処理が全てコントローラに書かれていて可読性が損なわれている状態。
どこの変数が、どこに影響を及ぼすかわからない状態があったりしていました。
「同じような処理を同じ記述にする」ことをやりつつService層を導入しました。
パロニムでService層はAppServiceを継承する形で各Serviceを実装しました。
コントローラからはnewすれば普通にserviceのメソッドを使えます。
Serviceである理由は下記の2点からです。
・newすればどこからでも柔軟に使える(componentはコントローラからしか使いにくく、Cakeとしての縛りが大きい)
・componentはそもそも「コントローラー間で共有されるロジックのパッケージです。(公式ドキュメントより)」なので単体のビジネスロジックを書く場所として最適な使い方ではない。
そして少しわかりにくくて迷いますが、Service層の作り方としては
・Serviceの考え方の基本はコントローラと対になる、
・または役割が明確な内部ロジックがある。
でコーディングしていきました。
そのほか引用させていただきますが、モジュールの結合度を意識できたらBESTです。テストが楽になります。
componentは
「そもそもがコントローラの処理なんだけども共通化したい。」ような処理を書くようにできたらなって感じです。
namespace App\Service;
use Cake\ORM\Table;
use Cake\ORM\TableRegistry;/**
* serviceクラスの基底クラス
* クラス共通の実装はここに記載
*/
class AppService
{
protected $_app;
/**
* 実行サービスのリソース
* 自動取得にするためにはサービスの命名を[テーブル名]+Serviceにすること
*/
// 抽出元するリソース名
private $tableName;
// 自動取得したテーブルレジストリ // 1.
private $tableRegistry; public function __construct($_app = null)
{
$this->_app = $_app; // 2. // サービスに対応するテーブル名を取得する
$path = explode("\\", get_class($this));
$this->tableName = str_replace("Service", "", $path[count($path)-1]);
// レジストリから取得したクラスがサービスに対応するtableクラス
$class_name = sprintf("App\\Model\\Table\\%sTable", $this->tableName);
if (class_exists($class_name)) {
$this->tableRegistry = TableRegistry::get($this->tableName);
} }
}
- session操作やログインユーザの取得は一覧性のためにメインストリーム(コントローラ)でやりたいので、serviceには$_appとして引数で渡せるようにしました。
- いちいちservice内から同じリソースのTableクラスを取得するのは面倒だったので暗黙的に同じリソースのTableクラスがある場合は取得しちゃいました
まずはAppServiceを継承したserviceクラスを各コントローラ毎に作ってガンガン細かなメソッドを切り出し。
メソッドを切り出すだけでコントローラが半分くらいの行数になり、可読性、一覧性が出てきます。
ユーザのグループ追加の部分
before
public function add($tab_menu=0, $id=0)
{
$this->autoRender = false;
// データチェック
$post_data = $this->request->getData();
// 戻り先セッションチェック
if ($this->Session->check(self::SESSION_BACK_USER) === true) {
$this->Session->delete(self::SESSION_BACK_USER);
}
// 遷移元からpost値のチェック
$post_data['tab_menu'] = $tab_menu;
// グループ
if((int)$tab_menu == (int)Configure::read("SELECT_TAB_MENU_GROUP")){
$check_array = ['groupId', 'groupName', 'groupRole', 'groupGaLink', 'groupNote'];
}
// ユーザー
else {
$check_array = ['userId', 'groupId', 'userName', 'userMail', 'userPass', 'userNote'];
$this->Session->write(self::SESSION_BACK_USER, 1);
if (array_key_exists('userGrpId', $post_data) === true) {
$this->Session->write(self::SESSION_ERR_UGID, $post_data['userGrpId']);
}
}
// 配列チェック
$insert_data = array();
foreach($check_array as $row) {
if (array_key_exists($row, $post_data) === false) {
$this->_log("not exits key [".$row."]", "debug");
if ((int)$id === 0) {
$this->Flash->error(MSG_USER_ADD_ERROR);
} else {
$this->Flash->error(MSG_USER_SAVE_ERROR);
}
$this->Session->write(self::SESSION_POST_DATA, $post_data);
return $this->redirect('/user-group/');
}
$insert_data[$row] = trim($post_data[$row]);
}
// ユーザー新規登録の場合、メールアドレス二重チェック
if (((int)$tab_menu == (int)Configure::read("SELECT_TAB_MENU_USER")) and ((int)$insert_data['userId'] === 0)) {
if ($this->Users->count_by_mail($post_data['userMail']) > 0) {
$this->Flash->error(MSG_USER_ADD_MAIL_ERROR);
$this->Session->write(self::SESSION_POST_DATA, $post_data);
return $this->redirect('/user-group/');
}
}
$connection = ConnectionManager::get('default');
$connection->begin();
try {
// グループ
if((int)$tab_menu == (int)Configure::read("SELECT_TAB_MENU_GROUP")){
$ret = $this->UserGroupService->saveGroup($insert_data);
}
// ユーザー
else {
$ret = $this->UserGroupService->saveUser($insert_data);
}
if (!$ret) {
$connection->rollback();
if ((int)$id === 0) {
$this->Flash->error(MSG_USER_ADD_ERROR);
} else {
$this->Flash->error(MSG_USER_SAVE_ERROR);
}
$this->Session->write(self::SESSION_POST_DATA, $post_data);
return $this->redirect('/user-group/');
}
if($tab_menu == Configure::read("SELECT_TAB_MENU_GROUP")){
if ((int)$insert_data['groupId'] === 0) {
$success_msg = $insert_data['groupName'].MSG_GROUP_ADD_FINISH;
} else {
$success_msg = $insert_data['groupName'].MSG_GROUP_SAVE_FINISH;
}
} else {
if ((int)$insert_data['userId'] === 0) {
$success_msg = $insert_data['userName'].MSG_USER_ADD_FINISH;
} else {
$success_msg = $insert_data['userName'].MSG_USER_SAVE_FINISH;
}
}
$connection->commit();
} catch (Exception $e) {
$this->_log($e->getMessage(), "error");
$connection->rollback();
if ((int)$id === 0) {
$this->Flash->error(MSG_USER_ADD_ERROR);
} else {
$this->Flash->error(MSG_USER_SAVE_ERROR);
}
$this->Session->write(self::SESSION_POST_DATA, $post_data);
return $this->redirect('/user-group/');
}
$this->Flash->success($success_msg);
return $this->redirect('/user-group/');
}after(現在のアクション)
新しいロールベースアクセス等が入っているため若干機能が増えていますが、それでも以前よりも安心できるコードかと思います。
バリデーションチェックはUserGroupService->checkForAddに固めてあります。
public function add()
{
$this->request->allowMethod("post");
$this->autoRender = false;
// データチェック
$post_data = $this->request->getData();
// 親グループがdisabledで来ない場合は代理店なので自分を入れる
if (!isset($post_data['parentGroupId'])) {
$post_data['parentGroupId'] = $this->_app["group_id"];
}
// 作成時のチェック
$owner_list = $this->UserAuthComponent->getOwnerList($this->permission_type, $this->_user["group_id"], $this->_user["id"]);
$error_msg = $this->UserGroupService->checkForAdd($post_data, $owner_list);
if ($error_msg) {
$this->Flash->error($error_msg);
$this->Session->write(self::SESSION_POST_DATA, $post_data);
return $this->redirect(['controller' => 'userGroup', 'action' => 'index']);
}
$connection = ConnectionManager::get('default');
$connection->begin();
try {
$this->UserGroupService->add($post_data);
$success_msg = $post_data['groupName'].MSG_ADD_CREATE_FINISH;
$connection->commit();
} catch (\Exception $e) {
$this->_log($e->getMessage(), "error");
$connection->rollback();
$this->Flash->error(MSG_ADD_ERROR);
$this->Session->write(self::SESSION_POST_DATA, $post_data);
return $this->redirect(['controller' => 'userGroup', 'action' => 'index']);
}
$this->Flash->success($success_msg);
return $this->redirect(['controller' => 'userGroup', 'action' => 'index']);
}DRYの原則を基本無視している
同じようなロジックが点在していました。
例えば下記のようなログ出力のロジックはまとめてAppServiceに実装してしまいましょう。
public function _log($msg, $level = "debug")
{
$debug = debug_backtrace();
$user_id_str = $this->_app["user_id"] ? " user_id=".$this->_app["user_id"] : "";
$message = $debug[1]["class"]."::".$debug[1]["function"]."(".$debug[0]["line"].") ".$msg.$user_id_str." ";
Log::write($level, $message);return true;
}
クエリビルダ周りもまとめれるものは複数あるはずなので共通のメソッドを作成しちゃいます
/**
* idでデータを取得する
* @param int $id
* @param boolean $contains_deleted
* @return \Cake\Datasource\EntityInterface|array|NULL
*/
public function finds($id, $exists_deleted = true)
{
$param = [
'id' => $id,
];if ($exists_deleted) {
$param['deleted'] = self::DELETED_FALSE;
}return $this->find()->where($param)->first();
}/**
* idで件数を取得する
* @param int $id
* @param boolean $contains_deleted
* @return number
*/
public function countById($id, $contains_deleted = true)
{
$param = [
'id' => $id,
];if ($contains_deleted) {
$param['deleted'] = self::DELETED_FALSE;
}return $this->find()->where($param)->count();
}
定数もハードコーディングしないでしっかりModelに書きましょう。
Tableに書いております。
Entityを使うようにリファクタをした方がしっかりとCakeっぽくなるんですが、費用対効果の観点から次のリファクタまで取っていております。
class ContentsTable extends BaseTable
{
// コンテンツのタイプ
const TYPE_VIDEO = 0;
const TYPE_MAGAZINE = 1;
const TYPE_STORY = 2;
const TYPE_STORY_BRANCH = 3;
}その他ロジックはファットコントローラーを防ぐためと、使い回しできるように考慮して細かい単位で(一つのタスク)でメソッドに分割するなど、基本的部分はまだまだ足りていません、、、
例えばこれとか。
/**
* 視聴完了率タイムライン、総再生時間、ユーザ総視聴時間を返却する
* @param array $time_structure
* @param array $context
* @param int $total_play_time
* @return number[]|string[]
*/
public function getViewingCompletionRateAndSummaryTime(array $time_structure, array $context, int $total_play_time)
{メソッドの役割は一つが理想ですが、ここではAndがあることから明らかですが、2つあります。
まだまだリファクタの余地はあります。
最後に
なぜリファクタをしたのか？しているのか？
下のスクショは以前技術ブログにも投稿してくれたマルチメディアエンジニアの池田さんのslack投稿のスクショです
本記事はこちら
だいたい最初に直面する問題としては
・修正点がどこに影響するかわかりにくいためバグが増える
・修正が多くなり、コストがかかり、エンジニアのモチベが下がる
ようなことが表面かします。
自分が業務委託で担当した1年9ヶ月前は、
機能追加ができなくはないが、ちょっと億劫になる感覚はあり、常に修正の不安がつきまとっていました。
その理由は上記であげたダークサイド化のためです。
放っておくとマズイと直感的に感じたので自らリファクタに取り掛かりましたが、それから1年以上が経過し、新規機能も続々と追加されています。(幸いにもビジネスが急成長しているため)
しかし弊社は技術を重んじる文化があり、日頃から意識的にリファクタの工数も入れて改修を進めていける状態です。
これから弊社で働くかもしれないエンジニアもそうでないエンジニアも常日頃から自分のコードがどのように周囲に影響を(将来的にも)及ぼすことになるのかを意識して開発に取り組んでいきましょう！
以上です！
ありがとうございました！
