本記事は、Fusic Advent Calendar その２の17日目の記事です。
昨日の記事は、@nmbakfm によるActiveStorageで恒久的なURLを取得するでした！
はじめに
2019年ももう終わりですね。
私はこの4月からWeb業界に入りました。
HTML/CSS/JSの研修から始まり、
CakePHPのドキュメントを読みながらチュートリアルで学び、
5-6ヶ月間のOJTでは、経費立替申請を行える社内用Webシステムを作り上げるに至りました。
「ほ？DOMってなになに？」状態だった自分が、
今Laravel・Vueで開発が出来ているのも、研修で基礎をしっかり作り上げてもらったからです。
（感謝の極み）
さてさて、本記事では、
この半年間+αを振り返るために、研修期間で頂いたレビューの数々をただただ羅列していきます。
皆さんにもこういう右も左も分からないという時期があったのではないでしょうか？
（「こんなのお前だけや」という声はグッと堪えましょう。）
※ 基本的にCakePHP関連です。
頂いたレビューをコメント付きで羅列
OJT前（〜2019年6月）
「マイグレーションにはup/down以外にもchangeがあるよ」
存在は知っていたものの違いが分からず、とりあえずup/downで書いてたところ、
このようにとても丁寧にコメントを書いて頂きました。
こんなに時間を使って書いていただけるのありがたいですね🙇♂️
「コメントアウトしてはダメですー」
当時csrfが何かよく分かっていなかったので、
ajax通信でcsrfが原因のエラーになったとき、Cake側のcsrfミドルウェアを切って「やったー通信できたー:D」とか言ってました。
正しくは、もちろんcsrf対策のミドルウェアは切らず（笑）、ajax通信にcsrfトークンを埋め込んであげることですね。
懐かしい
（Cakeのドキュメントもうちょっと書いてくれ。。ミドルウェア - 3.8）
「toList()は、ただの配列になってしまうので、ここでは正しくありませんー」
toList()では、idではなく配列の添字が入ってしまいます。
ここはfind(‘list’)にkeyFiledとvalueFieldを設定するのがよさそうですね。
OJT期間（2019年6月〜10月）
「toArrayって必要なのかな？」
恥ずかしい限りですが、一番多く頂いたコメントがこれですｗ
開発中にデータの確認をするときに使ったまま、よく残してしまっていました。
Cakeではクエリをviewに渡すと、勝手に良い具合にSQLを発行してデータを取り出しますので、特別必要でない限りはフレームワークに任せましょう。
そして、debugやconsole.logなどと同様に、コミット前にdiffでしっかり確認しましょう。
「言語の関数を活用しましょう」
こちらはjQueryでチェックボックスの値を取得する箇所です。
最終的に`String(Number($(this).prop(“checked”)))`と置き換える事ができました。
存在もしくは活用法を知らないだけで、まだまだ便利な関数があるので、引き出しを増やしておく程度に全て目を通しておきたいですね。
私は一時期、手元にPHPの関数リファレンス本を横において開発していました。
様々なケースに対する実装例がまとめてあるので、たまに見ると面白いです。
「view側にSession情報をそのまま読むのはイマイチだなー」
Cakeのviewであるctpファイル内です。
当時は、MVCが何なのかよく分かっていませんでした。
ここで言われているのは、Viewは受け取ったデータを見せるだけでデータの加工とか他の処理はModel/Controllerでしましょうね、ということです。
このコメントを頂いてからMVCについて少しずつ理解が出来てきました。
「この辺の判定はentityにまとめるとよいと思います。」
CakeはModelがTableクラスとEntityクラスに分かれており、処理の対象によって書き分ける事ができます。
例えば、このシーンであるレコードに対する判定を作るとすると、Entityに↓のようなgetter(アクセサー)を書くと良いと思います。
protected function _getIsReapplyable()
{
return
/* 条件１ */ ($this->application_status !== ApplicationsDefine::STATUS_RETURNED)
&&
/* 条件２ */ (!$this->admin_submit_flg);
}
もっというと、
・プロパティを利用する場合は、$this→properties[‘field name’]のように書く
・条件１も _getIsReturned()とgetter化する
とより良いかなと思います。
ちなみに、当時getterは仮想的なカラムを定義するものだよと言われてもよく意味が分からなかったのですが、
メソッドであるにも関わらずプロパティにアクセスするように$this→is_reapplyableと書けるからだと少しあとに理解しました。
当時はこういう点もあまりよく分かっていなかったのです。
「これは『なぜ』これを返したいかを書いておいたほうがいいです。」
↑の処理を満たすgetterを作ることは簡単です。
しかしメソッド名やDocの内容によっては、このメソッドが何のために存在しているのか分からなくなるときがあります。
単純なステータスの判定なら必要ないと思いますが、このシーンでいう「申請中」or「締め」の状態は結局`何を意味するのか`を明確にするのが大切です。
例として`「申請中」or「締め」状態： 編集・削除可能`など用途が想像できるコメントを添えておくとよさそうですね。
「これはfirst()で止めてしまうと、データがなかった時のことが検討されていないです。」
(もしtarget_month_idがないデータだったら、この行の時点でnullになり、→sumでPHPエラーが発生する)
データを取り出す時は、必ず存在するデータか存在しない可能性のあるデータかに注意する必要があります。前者の想定で仮にデータが無かった場合はシステムエラーであるため、しっかりExceptionを出しましょう。
「html でコメントアウトすると、ユーザーにコメントが見えてしまいますので、phpでコメントを書きましょう」
つまり、↓こういうことです。こういうの大事ですね。
<?php /* 対象月表示*/ ?>
「cdn からjs や css をとってくる場合は、href=“//cdnjs.cloudflare.com/…” みたいに書くのが一般的です。」
こちらのリンクの記述はselect2の[Installation](https://select2.org/getting-started/installation)ページから持ってきただけだったので、当時こういった知識は全くありませんでした。省略することでプロトコルに柔軟になります。
ただし、[Google的には](https://google.github.io/styleguide/htmlcssguide.html#Protocol)現在は省略するのは非推奨とのことですね。
「これController側の処理いらない？」
view側でボタンの表示に判定を付けていても、制御判定はController側に必須です。
複数ウィンドウから操作されることを想定すると、理由が明白ですね。
validationに関してもそうですが、view側のこういった制御判定は`UXのため`という考え方で間違っていないかと思います。
おわりに
まだまだ無数にありますが、上記のアドバイスらのおかげで考え方や視野が広がりました。
皆さまも今年一年をGithubで振り返ってみるのはいかがですか？
振り返りで何かまた得られるものがあるのではないかと思います。
また、本記事を読んで、自分もこんな時があったなと懐かしい思いにふける一方、
新人のレビューをする際には、こういう点が分かっていないんだろうな、という何かのヒントになればいいなと思います。
それでは良いお年を。
明日は @seike460 です！お楽しみに！