「誰にとってもわかりやすい、役割が明確な関数を作る」ことは、難しい技術、それも超難しい技術なんだと、最近あらためて感じます。
今日も偉そうにリファクタリングについて書いていきます。もしリファクタリングについて興味がある方は、リーダブルコードやリファクタリングなどの書籍をぜひ読んでみてください！本が好きな方は、ドメイン駆動開発もおすすめです！
今日はこちらの checkinSerchPoint をリファクタリングしていきます！
<?php
namespace App\Models;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use App\Models\Utility\LatLngUtil;
use Carbon\Carbon;
/**
* お気に入り地点に関するクラス
*/
class SearchPoint extends Model
{
use HasFactory;
const UPDATED_AT = null;
protected $dates = ['visited']; // DataTime型として扱うカラム
/**
* ユーザーIDからお気に入り地点を取得する関数
*
* @param [int] $user_id
* @return お気に入り地点のコレクション
*/
public static function getSearchPointsByUserId($user_id)
{
$result = SearchPoint::where('user_id', $user_id)->orderBy('id', 'asc')->get();
return $result;
}
/**
* 引数で受け取った緯度経度から100m以内のお気に入り地点をチェックインする
*
* @param int $user_id
* @param double $lat 緯度
* @param double $lng 経度
* @return void
*/
public static function checkinSerchPoint(int $user_id, float $lat, float $lng)
{
// お気に入り地点一覧を取得
$search_points = SearchPoint::getSearchPointsByUserId($user_id);
foreach($search_points as $search_point){
// 引数の緯度経度とお気に入り地点の緯度経度から距離を計算
$distance = LatLngUtil::calcDistanceByLatLng($lat, $lng, $search_point['lat'], $search_point['lng']);
// 計算結果はキロメートル単位なのでメートル単位に直す
$distance = $distance * 1000;
// お気に入り地点が100メートル以内にあるか判定
if($distance <= 100){
// visitedと同じ日か判定
if($search_point->visited->day == Carbon::now()->day){
// 10分以上滞在しているか判定
if($search_point->visited->diffInMinutes(Carbon::now()) >= 10){
$search_point->is_visited = 1;
$search_point->visited = Carbon::now();
}
}else{
// 違う日なら日付を更新
$search_point->visited = Carbon::now();
}
}else{
// 100m以内に入る→出る→また入るという流れを想定し、範囲から出た時点でvisitedを初期化する
// ただし滞在した日時を画面に表示することも考えてis_visitedが0のときだけ行う
if($search_point->is_visited == 0){
$search_point->visited = new Carbon('1000-01-01');
}
}
// 更新を保存
$search_point->save();
}
}
}わたしのリファクタリングの方針として、「リファクタリングをしないクラスに影響しない」ようにします。もし関数名を本当は変えたかったとしても、そこはぐっと我慢します。
今回の関数 checkinSerchPoint は、SearchPoint に CheckIn させたい気持ちが出ていると思います。しかし、PHPDocに書かれている日本語をきれいに書いてみると「引数で受け取った緯度経度から、100m以内にお気に入り地点があったら、その場所にチェックインしたことにする」ということをしたいんだなと読み取れます。
DeepLで翻訳するとこうです。
これだと長すぎるので、適当に短くすると「hasCheckedIn」とかになるのかもしれないですが、ちょっとうまいことまとまらないので checkinSearchPoint をそのまま使っていきたいと思います！
リファクタリングの内容としては、
- for文の中身やif文を短くまとめていきます。
- checkin（そこにいたことを保存する）ことが目的なので、保存することと判定処理をわけていきます
では始めましょう！
まずfor文の中の最初の3行で「引数の緯度経度とお気に入り地点の緯度経度から距離を計算」という関数を呼んで、取得したdistance を使って100m以内にあるか判定しています。ここを抜き出します。
もともと LatLngUtil を使っているので、ここに作ってしまいます。
<?php
namespace App\Models\Utility;
use App\Models\AdditionalUser;
use Illuminate\Database\Eloquent\Model;
use Carbon\Carbon;
use Illuminate\Support\Facades\Session;
use Auth;
/**
* 緯度経度を使った計算などを行う汎用クラス
*/
class LatLngUtil extends Model
{
// ～ 省略 ～
/**
* 緯度経度から距離を算出する
*
* @param float $lat1 地点1の緯度
* @param float $lng1 地点1の経度
* @param float $lat2 地点2の緯度
* @param float $lng2 地点2の経度
* @return double
*/
public static function calcDistanceByLatLng(float $lat1, float $lng1, float $lat2, float $lng2):float
{
// ～ 省略 ～
}
/**
* didComeWithinRange
* 引数1,2の緯度経度と引数3,4の緯度経度が引数 $range 以内に入っているか
*
* @param float $lat1 地点1の緯度
* @param float $lng1 地点1の経度
* @param float $lat2 地点2の緯度
* @param float $lng2 地点2の経度
* @param float $range 単位はメートル
* @return boolean
*/
public static function didComeWithinRange(float $lat1, float $lng1, float $lat2, float $lng2, float $range = 100.00) : bool
{
$distance = self::calcDistanceByLatLng($lat1, $lng1, $lat2, $lng2);
// 計算結果はキロメートル単位なのでメートル単位に直す
$distance = $distance * 1000;
// お気に入り地点が100メートル以内にあるか判定
if($distance <= $range) return true;
return false;
}
}この関数を呼びます
<?php
namespace App\Models;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use App\Models\Utility\LatLngUtil;
use Carbon\Carbon;
/**
* お気に入り地点に関するクラス
*/
class SearchPoint extends Model
{
use HasFactory;
const UPDATED_AT = null;
protected $dates = ['visited']; // DataTime型として扱うカラム
/**
* ユーザーIDからお気に入り地点を取得する関数
*
* @param [int] $user_id
* @return お気に入り地点のコレクション
*/
public static function getSearchPointsByUserId($user_id)
{
$result = SearchPoint::where('user_id', $user_id)->orderBy('id', 'asc')->get();
return $result;
}
/**
* 引数で受け取った緯度経度から100m以内のお気に入り地点をチェックインする
*
* @param int $user_id
* @param double $lat 緯度
* @param double $lng 経度
* @return void
*/
public static function checkinSerchPoint(int $user_id, float $lat, float $lng)
{
// お気に入り地点一覧を取得
$search_points = SearchPoint::getSearchPointsByUserId($user_id);
foreach($search_points as $search_point){
// お気に入り地点が100メートル以内にあるか判定
if(LatLngUtil::didComeWithinRange($lat, $lng, $search_point['lat'], $search_point['lng'])){
// visitedと同じ日か判定
if($search_point->visited->day == Carbon::now()->day){
// 10分以上滞在しているか判定
if($search_point->visited->diffInMinutes(Carbon::now()) >= 10){
$search_point->is_visited = 1;
$search_point->visited = Carbon::now();
}
}else{
// 違う日なら日付を更新
$search_point->visited = Carbon::now();
}
}else{
// 100m以内に入る→出る→また入るという流れを想定し、範囲から出た時点でvisitedを初期化する
// ただし滞在した日時を画面に表示することも考えてis_visitedが0のときだけ行う
if($search_point->is_visited == 0){
$search_point->visited = new Carbon('1000-01-01');
}
}
// 更新を保存
$search_point->save();
}
}
}最初の3行がすっきりしました！関数化することで、判定処理を独立することができました！独立したので、UnitTestも書きやすいと思います！
次に、if文の中身を変更していきます。ここは Carbon::now() が連発されていますので、引数に Carbon を指定して、1回で済むようにしたいですね。setVisited を作って呼ぶように変更します。
<?php
namespace App\Models;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use App\Models\Utility\LatLngUtil;
use Carbon\Carbon;
/**
* お気に入り地点に関するクラス
*/
class SearchPoint extends Model
{
use HasFactory;
const UPDATED_AT = null;
protected $dates = ['visited']; // DataTime型として扱うカラム
/**
* ユーザーIDからお気に入り地点を取得する関数
*
* @param [int] $user_id
* @return お気に入り地点のコレクション
*/
public static function getSearchPointsByUserId($user_id)
{
$result = SearchPoint::where('user_id', $user_id)->orderBy('id', 'asc')->get();
return $result;
}
/**
* setVisited
*
* @param SearchPoint $search_point
* @param integer $minutes
* @param Carbon|null $now
* @return void
*/
public static function setVisited(SearchPoint &$search_point, int $minutes = 10, ?Carbon $now = null)
{
if(empty($now)) $now = Carbon::now();
// visitedと同じ日か判定
if($search_point->visited->day == $now->day) {
// 10分以上滞在しているか判定
if($search_point->visited->diffInMinutes($now) >= 10){
$search_point->is_visited = 1;
$search_point->visited = $now;
}
} else {
// 違う日なら日付を更新
$search_point->visited = $now;
}
}
/**
* 引数で受け取った緯度経度から100m以内のお気に入り地点をチェックインする
*
* @param int $user_id
* @param float $lat 緯度
* @param float $lng 経度
* @return void
*/
public static function checkinSerchPoint(int $user_id, float $lat, float $lng)
{
// お気に入り地点一覧を取得
$search_points = SearchPoint::getSearchPointsByUserId($user_id);
foreach($search_points as $search_point){
// お気に入り地点が100メートル以内にあるか判定
if(LatLngUtil::didComeWithinRange($lat, $lng, $search_point['lat'], $search_point['lng'])){
self::setVisited($search_point);
}else{
// 100m以内に入る→出る→また入るという流れを想定し、範囲から出た時点でvisitedを初期化する
// ただし滞在した日時を画面に表示することも考えてis_visitedが0のときだけ行う
if($search_point->is_visited == 0){
$search_point->visited = new Carbon('1000-01-01');
}
}
// 更新を保存
$search_point->save();
}
}
}最後のif文は else if にまとめられるので、まとめちゃいます
<?php
namespace App\Models;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use App\Models\Utility\LatLngUtil;
use Carbon\Carbon;
/**
* お気に入り地点に関するクラス
*/
class SearchPoint extends Model
{
use HasFactory;
const UPDATED_AT = null;
protected $dates = ['visited']; // DataTime型として扱うカラム
/**
* ユーザーIDからお気に入り地点を取得する関数
*
* @param [int] $user_id
* @return お気に入り地点のコレクション
*/
public static function getSearchPointsByUserId($user_id)
{
$result = SearchPoint::where('user_id', $user_id)->orderBy('id', 'asc')->get();
return $result;
}
/**
* setVisited
*
* @param SearchPoint $search_point
* @param integer $minutes
* @param Carbon|null $now
* @return void
*/
public static function setVisited(SearchPoint &$search_point, int $minutes = 10, ?Carbon $now = null)
{
if(empty($now)) $now = Carbon::now();
// visitedと同じ日か判定
if($search_point->visited->day == $now->day) {
// 10分以上滞在しているか判定
if($search_point->visited->diffInMinutes($now) >= 10){
$search_point->is_visited = 1;
$search_point->visited = $now;
}
} else {
// 違う日なら日付を更新
$search_point->visited = $now;
}
}
/**
* 引数で受け取った緯度経度から100m以内のお気に入り地点をチェックインする
*
* @param int $user_id
* @param float $lat 緯度
* @param float $lng 経度
* @return void
*/
public static function checkinSerchPoint(int $user_id, float $lat, float $lng)
{
// お気に入り地点一覧を取得
$search_points = SearchPoint::getSearchPointsByUserId($user_id);
foreach($search_points as $search_point){
// お気に入り地点が100メートル以内にあるか判定
if(LatLngUtil::didComeWithinRange($lat, $lng, $search_point['lat'], $search_point['lng'])){
self::setVisited($search_point);
}else if($search_point->is_visited == 0) {
// 100m以内に入る→出る→また入るという流れを想定し、範囲から出た時点でvisitedを初期化する
// ただし滞在した日時を画面に表示することも考えてis_visitedが0のときだけ行う
$search_point->visited = new Carbon('1000-01-01');
}
// 更新を保存
$search_point->save();
}
}
}時間がなければこのくらいで！
もっと本当なら抜本的に変更したいところですが、まずはいくつかの処理が単体でテストできるようになったのでよしとしましょう！
とにかく、リファクタリングの目的は、1つの関数に対して役割を減らしていくことだと私は思います。今回リファクタリングの例にあげた checkinSerchPoint も決して悪くなかったです。ただ、UnitTestを作ることを考えると、かなり難解なテストが必要になっていました。条件が減れば減るほどUnitTestは楽になるためです。 checkinSerchPoint のUnitTestを全分岐網羅しようとすると
- getSearchPointsByUserId でデータがある user_id と無い user_id を用意する
- calcDistanceByLatLng で distance が 99, 100, 101 のデータを用意する←境界値
- distance が100以内のデータの中で、
$search_point->visited->day == Carbon::now()->dayがtrueとfalseのデータを用意する$search_point->visited->day == Carbon::now()->dayがtrueのデータの中で、$search_point->visited->diffInMinutes(Carbon::now()) >= 10がtrueとfalseのデータを用意する
- distance が100より大きいデータの中で
$search_point->is_visited == 0のデータを用意する
- getSearchPointsByUserId でデータがある user_id は、1件と複数件のデータがあるものを用意する←forループの境界値
- どれにも該当しないデータを用意する
これだけ必要です。データパターンとしては、10パターンくらい必要かなと思います。
リファクタリング後は、checkinSerchPoint のUnitTestに必要なデータはもっと減りました
- getSearchPointsByUserId でデータがある user_id と無い user_id を用意する
LatLngUtil::didComeWithinRangeが true と false のデータを用意する$search_point->is_visited == 0のデータを用意する- それ以外のデータを用意する
- getSearchPointsByUserId でデータがある user_id は、1件と複数件のデータがあるものを用意する←forループの境界値
- どれにも該当しないデータを用意する
これなら、5パターンくらいのデータで済みそうです！半分になりました！
もう少し時間をかければ、もっと減らせそうです。
リファクタリングの目的は、保守しやすくすること、ドメインに沿った設計に変更することですので、正解は毎回違いますが、今回はUnitTestがやりやすくなったかなと思います！
