目次
1.はじめに
2.laravelコードレビュー
3.jsコードレビュー
4.その他コードレビュー
1. はじめに
入社して1ヶ月が経ちました。今回が初投稿になります。
今回、研修で受けたコードレビューについてまとめたいと思います。
ちなみに言語・フレームワークは、「Laravel(PHP)と、jQuery（JavaScript）」です。
これから、エンジニアになられる方には参考になる部分があるかと思います。
なぜ書こうと思ったか？
- 同じミスを2度としないように、メモに残しておいて確認するため。
- 最初に指摘されるミスは大体似ると考えたため、今後入社するであろう方にも参考に。
2. 【Laravel】コードレビュー
○【Laravelルーティング】api用のルートを使う
全て、web.phpに書いていたことを指摘されました。
apiの部分はapi.phpに記載する様に修正
コントローラーのネームスペースも、
- namespace App\Http\Controllers\Auth
+ namespace App\Http\Controllers\Api;
○return：何か返さないといけない
ajax通信で、laravelからレスポンスを返すとき
修正前
return response()->json();
何も返さないと、HTTPステータスコードのみでの判断になってしまうため、apiとしては不適切
修正後
return response()->json([
'code'. => 200,
'message' => 'ok'
]);
○【laravel】ログイン後に、sessionIDをregenerate（再発行）してあげる
Auth::login();
$request->session()->regenerate(); // sessionID再発行
なぜセッションIDを再発行するのか？
- セッションフィクセーション対策のため（セッションID固定化攻撃をされないため）
対策
- ログイン前とログイン後でセッションIDを変える
- ログイン成功時にセッションIDを再発行する
○【コントローラメソッド名】showでは他のページの描画時メソッド名に困る
- public function show()
+ public function index()
1つのコントローラーで1つのページを描画する前提なら、show()でもいいが
他のページ（詳細ページや、一覧ページ）を描画する場合、これでは後々困る。
indexとかページ名にしておく
○負荷対策をしましょう
全件取得はNG。
初歩的なミス、、、
修正前
return $this->get();
修正後
return $this->paginate(20);
その他参考に以下記事がかなりためになりました。（他の負荷対策に役立つかも)
○HTTPのリクエストパラメータを付加するとき
セキュリティの関係上、
攻撃者から、意図しないデータが送られてくる可能性もあります。
対策
ユーザー入力を少なくする。
どうしてもパラメータを渡す場合は、バリデーションを入れてDBの存在チェックをする。
○【Route】認証はauth:sanctum
○バリデーションは処理と処理の間にそれぞれ設置する
「新規登録の際、すでにユーザーが存在する場合はどうするのか？」
と、指摘が。
修正前
'email' => 'unique:users,email',
public function register($userInfo)
{
return User::create($userInfo);
私「バリデーションにunique入れて一意にしている。入り口が防げているからこれで良いのでは・・・？？」
だがこれではいけないみたいです。
理由は？
仮に、コントローラー以外から処理する（Serviceを通して、別の開発者が使用など）場合にそのまま処理が通ってしまうから
フォームリクエストのバリデーションでコントローラーの処理は止めれてが、それ以外のServiceやmodelからの直接の処理はそのまま通る。
修正後
マイグレーションファイルを修正（一意のデータ）
$table->string('email')->unique() // unique追加
○【PHP】（当現場において）スネークケースを使う場面は？
修正前
// ページネーション（リスト単位）
'pagingSize' => 20,
→配列の添字のように、スネークケースにすると違和感がないとの指摘を。
修正後
// ページネーション（リスト単位）
'paging_size' => 20,
○【blade】view側で重い関数を呼び出さない
修正前
$followService->isFollowed($follower->user_id)
view側（blade）で、引数の情報を持つかどうかをDBから取得している処理です。
これをループで回していたこともあり、数が増えることで処理が重くなるという指摘を受けました。
viewの役割はあくまで、データを表示させたりする、入出力したりです。
裏側で処理して結果だけを返すように修正。
○【ORM】
修正前
public function getFollowedRow($followeeId)
行はデータベースレイヤの概念。
ORMでオブジェクトの概念へと変換しているのでメソッド名にRow入るのは・・・「△」
修正後
public function getFollowedUser($followeeId)
3. 【js】コードレビュー
○【webpack】必要なページにだけ読み込む様にする
jsを全てapp.jsにまとめてバンドルさせていたので指摘を受けました。
読み込む必要のないページにもjsが読み込まれることはパフォーマンス的に良くないということで。
指摘コード
全てのページをまとめてapp.jsにまとめてバンドルさせていた。
require('./components/header.js');
require('./components/auth_modal.js');
require('./components/follow_btn.js');
require('./components/user_menu.js');
require('./components/register.js');
修正後
共通部分はapp.jsのまま
require('./components/header.js');
その他は、バンドルするファイルを分けて
mix.js('resources/js/app.js', 'public/js')
.js('resources/js/top.js', 'public/js')
.js('resources/js/components/auth_modal.js', 'public/js/components')
// 以下省略
それぞれ、bladeへ表示
// 子ブレード
@push('script')
<script src="{{ mix('js/top.js') }}"></script>
@endpush
// 親ブレード
@stack('script')
<script src="{{ mix('js/app.js') }}"></script>
○【ajax】データの送信方法にserialize()を使う
serialize()
入力された全てのElementを文字列のデータにシリアライズする。
http://semooh.jp/jquery/api/ajax/serialize/+/
指摘コード
このように、送るform内のvalue値取得したデータを送信していた。
$.ajax({
// 省略
data: {
name : $('#registerForm input[name="name"]') .val(),
email : $('#registerForm input[name="email"]').val(),
password : $('#registerForm input[name="password"]').val(),
password_confirmation: $('#registerForm input[name="password_confirmation"]').val(),
},
})
修正後
シリアライズが可能で一つの文字列として表現して、送信→DB保存。
また、必要な時にDBからでシリアライズして取り出す
let $form = $(this).parents('form');
$.ajax({
// 省略
data: $form.serialize()
})
注意
ただ、シリアライズするとデータ量が増えてしまう場合もあるため、使うときは状況に応じて。
○ajaxで送信するなら、formタグを使わずPOST出来る事を知った
formでactionを使って、preventDefaultしていると矛盾が生じる。
formタグは使わずに、preventDefaultも使わずにPOSTする事も出来る
ただ、HTML標準は守りましょう。
○【ajax】APIのURLはできるだけ隠蔽する
formで送信するときに、action属性をイジられるとどんなapiでも送ることができる。
jsではurlなどを改変しようと思えばなんとでもなる。
ただ、laravelの場合sanctumで認証されたセッションしかAPIが使えないないので一応は安心。
4. 【その他】コードレビュー
○【見た目】位置を揃える
修正前
type : 'POST',
dataType: 'json',
url: '/api/register', // ズレてる
修正後
type : 'POST',
dataType: 'json',
url : '/api/register',
○【命名】メソッド名、クラス名の付け方がよくない
これは、最初はみんな言われるのかも。。。
○【よくやってしまう】使っていないものは消す
// import './bootstrap'
コメントアウトとか、使ってないパッケージとか。いらないなら消しておく。
いる場合は、コメント書いておく。
あとで使うとか、理由があって残す場合は、コメント残す
// TODO: あとで使うから残しておく
// import './bootstrap'
○コメントについて
●実際の挙動に整合性を持たせる
コメントを書くなら「コード内容と、コメント内容」正しいかどうか？
pushする前にもう一度確認する
修正前
// みかん
apple
修正後
// りんご
apple
●APIにはAPIってつけてあげる
- // ログインコントローラー
+ // ログインAPI コントローラー
○シンプルに
修正前
$registerService->register([
'name' => $request->name,
'email' => $request->email,
'password' => Hash::make($request->password)
]);
if (!Auth::attempt(['email' => $request->email, 'password' => $request->password])) {
return response()->json([
'code' => 403,
'message' => 'ログイン出来ませんでした',
]);
}
return response()->json([
'code' => 200,
'message' => 'ログインできました',
]);
修正後
$user = null;
try {
$user = $registerService->register([
'name' => $request->name,
'email' => $request->email,
'password' => Hash::make($request->password)
]);
} catch (\Exception $e) {
Log::error($e);
return response()->json(['code' => 403, 'message' => 'ログイン出来ませんでした']);
}
dd($user);
Auth::login($user);
return response()->json(['code' => 200, 'message' => 'ログインできました']);
○ローカルと、リモートのversionはどんなものも揃えましょう
nodeのバージョンが違うだけで時間を無駄にしました。
○【DB】unsignedと、インデックス
【id】
①unsigned型にする
②indexを貼る
修正前
$table->id();
$table->bigInteger('user_id');
修正後
$table->id()->unsigned();
$table->bigInteger('user_id')->index();
○【Docker】ユーザーを分ける
実行ユーザーとrootユーザーは別にする。
なぜか？
セキュアにするため、実行ミスを防ぐ。