ども、エンジニア大家です。
数年前からソースコードのレビューをする側の人間になりましたが、
現場に入ったばかりかつ、経験があまりない方の場合に同じような事で躓いていることが多い印象を持ちました。
プログラミング教室や独学だと気が付かない内容なのかな?と思ったので、今回は記事にしてみました。
現場で学ぶプログラミング
新人さんにコードレビューで指摘した例で説明したいと思います。
差分に余計なものが含まれている
プログラミング以前の話になっちゃいますが、最近だとgitを利用してソースレビューをすることが多いですが、
明らかにタスク範囲外の差分が出ていました。
それを指摘したところ、
新人さん
的なことを言われたことがありました。
原因は不明ですが、おそらく開発の初期で段階でマージ先とは関係ないブランチから開発ブランチを作成したと思われます。
そして、差分の内容はマージ先のブランチ(この場合developブランチ)にマージされてしまうことをどうやら知らないようです。
そのことを教えてあげて「修正対象だけ差分が出るようにやり直してください」と伝えると、
新人さん
と不満気なお答えを頂きました。
in_arrayは「型」で判定すべき
いきなり細かい内容になりますが、PHPでin_arrayを使った判定の場合、
第三引数にtrueを入れないと挙動がおかしくなることがあります。
詳細は以下を見てください。
参考 in_arrayを使うときは黙って第三引数を付けることQiita簡単に説明すると、比較する際に「型」まで見る必要があるので常に第三引数にはtrueを入れとけということです。
現場のルールだと第三引数はtrueを入れる事になっていた
ちょっと脱線しますが、私が居た現場のルールだと第三引数はtrueを入れる事になっていました。
☓in_array(1, $array)
○in_array(1, $array, true)
ですが、レビュワーが複数いまして、「第三引数を入れなくても問題がない場合」はそのルールを無視してレビューを通してる場合もあり、結果として記述が統一されてませんでした。
という状況でしたが私がルール通りに指摘すると、、
新人さん
的な文句を言われてしまいました。
SQLのSELECT文をループ内で複数発行している
これもケースバイケースではありますが、
ループの中で毎回SELECTするよりもループの手前でSELECTを一度行うと効率が良い場合があります。
foreach($rows as $row){
$member = $db->query("SELECT * FROM members WHERE id = " . $row['member_id']);
if($member['mail_address'] !== null){
...
}
}
ではなくて、
$member_ids = array_column($rows, 'member_id');
$members = $db->query("SELECT * FROM members WHERE id IN (" . implode(',', $member_ids) . ")"); //ループ手前で取得
$members = array_column($members, null, 'id');
foreach($rows as $row){
$member = $members[$row['member_id']]; //取得済みデータを使う
if($member['mail_address'] !== null){
...
}
}
のようにすれば会員テーブルへの問い合わせは一回で済みます。
SQLが複数回ではなく一回の方がDB負荷が少なくて済みます。
SQLのINSERT文をループ内で複数発行している
これまたケースバイケースですが、
INSERT文も同じテーブルに大量に追加する場合はバルクインサートにしたほうが無難です。
バルクインサート自体は大した話ではないですが、
切羽詰まった状態でレビュワーから指摘されたら嫌だと思うので今のうちに覚えちゃってください。
参考 SQLのインサートとバルクインサートQiitaおそらく開発現場で使ってるフレームワークのModelの親クラスで実装されているのではないでしょうか?
もしなかった場合かつ実装しろと言われたら、、頑張ってくださいw
汎用的な作りにするならば、以下の点も考慮されると良いです。
- テーブルのカラム定義がauto_incrementだと勝手に値が入るid値をあえて指定できるようにする
- フレームワークの設定で勝手に値が入る追加日created、更新日modifiedを指定できるようにする
SQLのUPDATE文をループ内で複数発行している
これもまたケースバイケースになってしまいますが、
前述のINSERT文と同様に一回のSQLにまとめることができます。
こちらもレビューでいきなり指摘されたら嫌だと思うので今のうちに知っておいてください。
参考 MySQLでバルクアップデートを実現するにはQiita上の参考サイトでいくつか方法が記載されてますが、
自分は「ELT()とFIELD()を使った方法」の経験があります。
これも開発現場で使ってるフレームワークのModelの親クラスで実装されているかもしれませんが、
もしなかったらご自身で実装してみてくださいw
汎用的な作りにするならば、
更新するレコードが多すぎる場合を考慮するために、
一回の最大処理レコード数を指定できるような仕組みが良いと思います。
まとめ
個人的な経験で恐縮ですが、新人の方がこれまでに多かった「躓きポイント」を記事にしました。
上記の内容は必須ではない箇所もありますが、もし指摘された場合は対応できるように準備だけはしておいてください。
開発者がレビューで指摘されて嫌なポイント
あと、開発者がレビューで指摘されて嫌なポイントは「将来的に運用側が困るから未然に防いどけ」的な内容だと思います。
正直そのままでも動くけど、「高負荷な時に障害になりうるから」という理由で対応させられます。
開発側としてはやっとの思いで実装したソースなのに修正を迫られると嫌な気分になりますが、
逆にそれさえこなしてベストを尽くしておけば責任の所在は他の人になるので、めげずに頑張ってください。