【技術全般】未経験〜これまでに受けた「コードレビュー」まとめ

はじめに

対象読者

• これから実務に入るにあたって技術面で意識すべきことが知りたい方

内容について

こちらの記事では、これまでに参画した現場で受けたコードレビューや、学んだことについて記載しています。
主に技術的なことについて記載されています。

言語や、フレームワークが異なれば理解できないことや、必要な知識としてマッチしないものがあるかと思います。
しかし、多少なりとも参考になることがあるはずです。

「重要度」の高いと思うものから記載していますので、
現場で求められているコードの書き方などが少しでも伝わればと思います。

また、各エンジニアによってレビューの内容は異なります。
同じ現場のエンジニアであっても真逆の指摘を受けることは多々あります。（実際に何度もありました。）
ここに記載されていることと全く別の指摘を受けることが今後もあると思います笑

その時は、現場に合わせるなり、自分の正しいと思うものを信じるなりしてください。

技術編

⭐️⭐️⭐️重要

○類似コードは不具合の原因に

同じコードは散らかさない。DRY原則です。

一度はコードレビューで指摘されるかと。。。

これは言語問わず意識することになると思います。

例えば、
モーダルウィンドウなどは、コンポーネントとして一つにまとめておきましょう。

○命名について

・メソッド名の付け方

どこの現場、どんな言語、関係なく指摘を受けるはず・・・

メソッド名は、動詞で始める

sample.php

private function getProject()
{
    // 例：動詞+名詞
}

・関数名の付け方

こちらも同様に、動詞で始める

指摘例

一般的すぎる名前で公開する事は少し問題。
クラス内のメソッドにするならこの名前でもいけれど、関数として公開するなら具体的な名前に。

sample.py

# 関数
def get_node

修正後

sample.py

def get_breadcrumb_tree

・変数名の付け方

必ず名詞で始める。

指摘例

sample.js

let openPageNum = 1;

修正後

sample.js

let openedPageNum = 1;
// もしくは
let currentPageNum = 1;

○使っていないコメントアウトは消す

指摘コード

return redirect()->route('top');
// return view('top');

修正後

return redirect()->route('top');

対処法

• どうしても後から使うから残しておきたい場合は、コメントで残しておく

return redirect()->route('top');
// return view('top'); // TODO: あとで追加する

• 「本当に全部コメント消したのか心配・・・」ってな場合

現場では、pushした後に、レビュアーに対してプルリクエストを送ることになります。
プルリクエストを送る前に、github上で差分を確認する癖をつけることでコメントの消し忘れがないのか簡単に確認できます。
是非、プルリクの前にもう一度確認を。

○数字・場所での命名はご法度

理由は？

仕様が変わった時、class名もその都度合わせる形で変更しないといけないから。
あと、どこのblockを指しているか分かりにくい。

例えば

<div class="block-1">
  <p class="left-text"></p>
  <p class="right-text"></p>
</div>
<div class="block-2">
  <p class="left-text"></p>
  <p class="right-text"></p>
</div>

仕様変更でblock1とblock2の位置を変更。
block1のleft-textと、right-textも入れ替える。
となった場合

<div class="block-2">
  <p class="right-text"></p>
  <p class="left-text"></p>
</div>
<div class="block-1">
  <p class="left-text"></p>
  <p class="right-text"></p>
</div>

順番が混沌としてきて、
例はシンプルですのでまだマシですけれど、実際はもっとカオスな状態になってしまいます。

対策例

<div class="userinfo-block">
  <p class="name"></p>
  <p class="age"></p>
</div>
<div class="detail-introduce-block">
  <p class="past"></p>
  <p class="future"></p>
</div>

これならblockが入れ替わっても、cssを変更するだけで済みます。

○【セキュリティ】バリデーションチェック時、情報を極力知らせない

例えば、メールアドレスとパスワードでログインするページにおいて。
ここで気をつける事としては、

• エラーメッセージに余分な情報を載せない
  • 「メールアドレスが異なります」「パスワードが異なります」など
  • 理由としては、攻撃者にとって有力な情報を与えてしまうからです。
  • ユーザにとって親切な情報でも、リスト攻撃や総当り攻撃を受けるリスクが高まります。

○push、プルリクの前に最終確認を

pushする前に、もう一度、修正は完了したのか？
pushした後も、GitHubの差分を確認しながら修正漏れはないか？など

コードレビューを受けての修正が想像以上に時間がかかるので忘れがちですが、
確認が最も大事です。

○関数の生成場所

関数内の関数は、親の関数の実行ごとに関数生成するのでパフォーマンスが悪い

対策

クラス関数にしておけば再利用できる

⭐️⭐️やや重要

○【HTML】inputタグのtype属性を使い分ける

受けた指摘

input type="email" だとブラウザ側でもバリデーションしてくれる

sample.html

- <input type="text" placeholder="メールアドレスを入力">
+ <input type="email" placeholder="メールアドレスを入力">

emailに限らず、html側で適切にtype属性を使い分けることが出来ます

○厳密比較算子（(例)!==）での、厳密な入力チェックは必要？？

これは、人によって指摘が異なります。

ただ、一般的には厳密比較算子を使うべきだと個人的には思います
（今の現場は、比較算子を使用するように指摘されますが、、、笑）

前提

入力チェックには、
厳密比較算子（(例)!==）でのチェックと、比較演算子（(例)!=）があります。

簡単に言えば、**型のチェックまで行うかどうか？**の違いです。

どちらを使うのが適切なのか？といった話になります。

厳密な入力チェックは必要が無い派

理由は？

パフォーマンスの観点から != で十分

- Route::currentRouteName() !== 'top'
+ Route::currentRouteName() != 'top'

厳密な入力チェックは必要派

異常な挙動をするため

例えば、以下の比較演算子だと、どちらもtrueを返します。

 1  == "1.0"; // true
"1" == "1.0"; // true

○【mysql】varcharを設定しても実はあまり意味がない

migration.php

$table->string('name', 20);

理由

上記の場合、20文字で設定しているが21文字送られてくれば1文字削られてDBに挿入される。

また、後に20文字以上へとDBのテーブル定義を変更したいとなった場合に、データ量が増えていたらそう簡単にはいかないから。

対策

アプリケーション側で制約をつけてあげれば良い

○【DBマイグレーション】外部キー制約をつけるか？つけないか？

おそらく、現場によって指摘は異なるでしょう。

制約あり派

• 大規模サービスでは、パーティションとかシャーディングが難しくなる

制約なし派

• 存在しない値の登録を防ぐことができるため、データの整合性が保てる

⭐️参考程度に

○【webpack】必要なページにだけ読み込む様にする

jsを全てapp.jsにまとめてバンドルさせていたので指摘を受けました。

読み込む必要のないページにもjsが読み込まれることはパフォーマンス的に良くないということで。

指摘コード

全てのページをまとめてapp.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のまま

app.js

require('./components/header.js');

その他は、バンドルするファイルを分けて

webpack.mix.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>

○終わりに

今回の内容は約4ヶ月間で受けたコードレビューの内容です。
中には賛否を生むものを含まれていると思いますが、これから実務に入られる方に少しでも参考になれば幸いです。

分かりにくい箇所、質問等あればお気軽にコメントください。

別のコードレビュー記事もございますすのでご覧ください。
【Laravel編】未経験〜これまでに受けた「コードレビュー」まとめ
【jQuery編】未経験〜これまでに受けた「コードレビュー」まとめ

最後まで、読んでいただきありがとうございました。