Help us understand the problem. What is going on with this article?

新卒がこの半年+αでもらったレビューを振り返る

本記事は、Fusic Advent Calendar その2の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があるよ」

Screen Shot 2019-12-17 at 11.08.55.png
存在は知っていたものの違いが分からず、とりあえずup/downで書いてたところ、
このようにとても丁寧にコメントを書いて頂きました。
こんなに時間を使って書いていただけるのありがたいですね🙇‍♂️

「コメントアウトしてはダメですー」

Screen Shot 2019-12-17 at 10.44.40.png
当時csrfが何かよく分かっていなかったので、
ajax通信でcsrfが原因のエラーになったとき、Cake側のcsrfミドルウェアを切って「やったー通信できたー:D」とか言ってました。
正しくは、もちろんcsrf対策のミドルウェアは切らず(笑)、ajax通信にcsrfトークンを埋め込んであげることですね。
懐かしい

(Cakeのドキュメントもうちょっと書いてくれ。。ミドルウェア - 3.8

「toList()は、ただの配列になってしまうので、ここでは正しくありませんー」

Screen Shot 2019-12-17 at 10.57.08.png
toList()では、idではなく配列の添字が入ってしまいます。
ここはfind(‘list’)にkeyFiledとvalueFieldを設定するのがよさそうですね。

OJT期間(2019年6月〜10月)

「toArrayって必要なのかな?」

Screen Shot 2019-12-17 at 22.06.28.png
恥ずかしい限りですが、一番多く頂いたコメントがこれですw
開発中にデータの確認をするときに使ったまま、よく残してしまっていました。
Cakeではクエリをviewに渡すと、勝手に良い具合にSQLを発行してデータを取り出しますので、特別必要でない限りはフレームワークに任せましょう。

そして、debugやconsole.logなどと同様に、コミット前にdiffでしっかり確認しましょう。

「言語の関数を活用しましょう」

Screen Shot 2019-12-17 at 22.50.12.png
こちらはjQueryでチェックボックスの値を取得する箇所です。
最終的にString(Number($(this).prop(“checked”)))と置き換える事ができました。
存在もしくは活用法を知らないだけで、まだまだ便利な関数があるので、引き出しを増やしておく程度に全て目を通しておきたいですね。

私は一時期、手元にPHPの関数リファレンス本を横において開発していました。
様々なケースに対する実装例がまとめてあるので、たまに見ると面白いです。

「view側にSession情報をそのまま読むのはイマイチだなー」

Screen Shot 2019-12-17 at 13.28.37.png
Cakeのviewであるctpファイル内です。
当時は、MVCが何なのかよく分かっていませんでした。
ここで言われているのは、Viewは受け取ったデータを見せるだけデータの加工とか他の処理はModel/Controllerでしましょうね、ということです。
このコメントを頂いてからMVCについて少しずつ理解が出来てきました。

「この辺の判定はentityにまとめるとよいと思います。」

Screen Shot 2019-12-17 at 13.06.35.png
CakeはModelがTableクラスとEntityクラスに分かれており、処理の対象によって書き分ける事ができます。
例えば、このシーンであるレコードに対する判定を作るとすると、Entityに↓のようなgetter(アクセサー)を書くと良いと思います。

protected function _getIsReapplyable()
{
    return
        /* 条件1 */ ($this->application_status !== ApplicationsDefine::STATUS_RETURNED)
        &&
        /* 条件2 */ (!$this->admin_submit_flg);
}

もっというと、
・プロパティを利用する場合は、$this→properties[‘field name’]のように書く
・条件1も _getIsReturned()とgetter化する
とより良いかなと思います。

ちなみに、当時getterは仮想的なカラムを定義するものだよと言われてもよく意味が分からなかったのですが、
メソッドであるにも関わらずプロパティにアクセスするように$this→is_reapplyableと書けるからだと少しあとに理解しました。
当時はこういう点もあまりよく分かっていなかったのです。

「これは『なぜ』これを返したいかを書いておいたほうがいいです。」

Screen Shot 2019-12-17 at 21.50.12.png
↑の処理を満たすgetterを作ることは簡単です。
しかしメソッド名やDocの内容によっては、このメソッドが何のために存在しているのか分からなくなるときがあります。
単純なステータスの判定なら必要ないと思いますが、このシーンでいう「申請中」or「締め」の状態は結局何を意味するのかを明確にするのが大切です。
例として「申請中」or「締め」状態: 編集・削除可能など用途が想像できるコメントを添えておくとよさそうですね。

「これはfirst()で止めてしまうと、データがなかった時のことが検討されていないです。」

(もしtarget_month_idがないデータだったら、この行の時点でnullになり、→sumでPHPエラーが発生する)
Screen Shot 2019-12-17 at 22.04.30.png

データを取り出す時は、必ず存在するデータ存在しない可能性のあるデータかに注意する必要があります。前者の想定で仮にデータが無かった場合はシステムエラーであるため、しっかりExceptionを出しましょう。

「html でコメントアウトすると、ユーザーにコメントが見えてしまいますので、phpでコメントを書きましょう」

Screen Shot 2019-12-17 at 22.17.12.png
つまり、↓こういうことです。こういうの大事ですね。

<?php /* 対象月表示*/ ?>

「cdn からjs や css をとってくる場合は、href=“//cdnjs.cloudflare.com/…” みたいに書くのが一般的です。」

Screen Shot 2019-12-17 at 22.19.43.png
こちらのリンクの記述はselect2のInstallationページから持ってきただけだったので、当時こういった知識は全くありませんでした。省略することでプロトコルに柔軟になります。
ただし、Google的には現在は省略するのは非推奨とのことですね。

「これController側の処理いらない?」

Screen Shot 2019-12-17 at 22.41.26.png
view側でボタンの表示に判定を付けていても、制御判定はController側に必須です。
複数ウィンドウから操作されることを想定すると、理由が明白ですね。
validationに関してもそうですが、view側のこういった制御判定はUXのためという考え方で間違っていないかと思います。

おわりに

まだまだ無数にありますが、上記のアドバイスらのおかげで考え方や視野が広がりました。

皆さまも今年一年をGithubで振り返ってみるのはいかがですか?
振り返りで何かまた得られるものがあるのではないかと思います。

また、本記事を読んで、自分もこんな時があったなと懐かしい思いにふける一方、
新人のレビューをする際には、こういう点が分かっていないんだろうな、という何かのヒントになればいいなと思います。

それでは良いお年を。

明日は @seike460 です!お楽しみに!

Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
Comments
No comments
Sign up for free and join this conversation.
If you already have a Qiita account
Why do not you register as a user and use Qiita more conveniently?
You need to log in to use this function. Qiita can be used more conveniently after logging in.
You seem to be reading articles frequently this month. Qiita can be used more conveniently after logging in.
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
ユーザーは見つかりませんでした