初心者のためのセルフレビューチェック項目

この記事について

新卒1年目のわたしがコードレビューで指摘された箇所を参考に、初学者がよく指摘されることとその対処法をまとめました。
どの言語にも共通することと、PHP / Laravel 限定のもの別にまとめてあります。
また、それぞれのミスについて、指摘されたくない度を星5つ満点で評価してみました 笑
レビューされるとき、レビューするときにこの記事に書いてあることが参考になればいいなと思います。

注意

• あくまでも弊社基準のレビューです
• VSCode 使っている人向けかもしれません

言語に関係ないイージーなミス

■ スペルミス

指摘されたくない度★★★★★
指摘されたら恥ずかしいやつ。

[対処法]

• VSCode 拡張機能　Code Spell Checker
  • スペルミスしてる単語を下画像のように波線でハイライトしてくれる

• 不安だったらググる癖をつける

■ インデントずれ

指摘されたくない度★★★★★
GitHub 上でレビューするとなかなか気付き辛い。

[対処法]

• VSCode 拡張機能　indent-rainbow
  • 下画像のような感じでインデントに色付けをしてくれる。また、ズレがある部分を赤く表示してくれる

■ 余分なスペースが入ってしまう

指摘されたくない度★★★★★
最初の頃、誰もがやってしまうミス。

[対処法]

• VSCode の setting.json に以下を追加

setting.json

"files.trimTrailingWhitespace": true,

■ ファイル末尾改行がない

指摘されたくない度★★★★★

[対処法]

• VSCode の setting.json に以下を追加

setting.json

"files.insertFinalNewline": true

レビュワーとして気付くには

• GitHub の FilesChanged でみると、末尾改行がないファイルは下記画像のようなマークがあるのですぐに気付ける

■ デバッグ、コメントアウトの消し忘れ

指摘されたくない度★★★★★
dump() など、デバッグコードを消さずに残してしまう。

[対処法]

レビュー出す前に確認をする。

• add する前にエディタ上で確認
• push 後に Github 上で確認

■ 使っていないメソッドの消し忘れ

指摘されたくない度★★★★☆

試しに書いてみたけど結局使わなかったメソッドを消し忘れてしまう。

[対処法]

レビュー出す前に確認をする。

• add する前にエディタ上で確認
• push 後に Github 上で確認

■ 厳密等価演算子 === を使う

指摘されたくない度★★★★☆

[対処法]

基本的にゆるい等価比較 == は使わないって決めてれば良さそう

■ ダサいメソッド名と変数名

指摘されたくない度★★★★☆

● 長すぎる

例えば、Images ディレクトリ内のファイルで「この写真はjpeg形式か」みたいなメソッドを作るとき。メソッド名はisJpegImage()にしなくても isJpeg() で十分伝わる

● 正しい命名規則でない

指摘されたくない度★★★★☆
推奨されているケースで書く

~PHP の場合~

• メソッド：キャメルケース
• 変数：キャメルケース
• 定数：大文字のスネークケース（コンスタントケース）

● 複数形であるべきところが単数形

配列につける変数名は複数形が良い。

$numbers = [1, 2, 3];

// ループ処理も複数形が好ましい
foreach($values as $value) {
  // 処理
}

但し、可算名詞と不可算名詞に気をつける。
例）data の複数形は datas ではない

● beforeXXX と afterXXX に注意

例えば、beforePostDate() というメソッドがあるなら、return されるものは PostDate を含まない日付が好ましい。afterPostDate() というメソッドでも、PostDate を含まない日付が return されるべき。

参考： beforeの日付は当日を含むか含まないか

[対処法]

• codic で検索してみる
  • 例えば日本語で「有効かどうか」と入力すると、is_valid とカラム名っぽいものが表示される。状況に合わせてキャメルケースにしたりする。
• 戻り値が bool 型のメソッドは hasXXX isXXX とかにするといい感じになる。
  • 但し、is + 現在形の動詞（isShow など）は英語的におかしいので避けるべき。

PHP / Laravel でありがちなミス

■ 使っていない use の消し忘れ

指摘されたくない度★★★★☆
Laravel あるあるレビューだと思う。
使ってないものがあってもエラー出ない上、なぜか気付かれにくいので、ずっと残り続けることがあって厄介。

[対処法]

• VSCode 拡張機能　phpcs
  
  • これを入れると、保存時に自動で未使用の use を削除してくれます。但し、修正したくない箇所も勝手に修正されてしまうことがあるので、各自設定が必要
  • こちらの記事→ VScodeでPHP CodeSnifferの設定をしたい時の手順 に設定方法が書いてあった
    　

■ N+1 が解決されていない

指摘されたくない度★★★★☆

####[対処法]
Laravel の Eagerロード で解決。
参考： https://readouble.com/laravel/6.x/ja/eloquent-relationships.html#eager-loading

そもそも N+1 になってることに気付くには？

Laravel Debugbar を導入すると、N+1 があると、画像のように N+1 Queries タブに数字が表示される。さらに、You should add 'with(partners)' のように追加するべきメソッドも表示してくれる。
Laravel Debuger は発行されたクエリの表示もできるので、すごく便利。

■ if文じゃなくてもかける

指摘されたくない度★★★☆☆

● メソッドチェーンで頑張れるとき

下記コードの①②は同じ結果になる。

sample.php

// ①
if ($this->isFemale) {
    return $Girls
        ->where(function ($query) {
            $query->....;

// ②
// when が使える！！
return $Girls
    ->when($this->isFemale, function ($query) {
        $query->where(function ($query) {
            $query->....;

• when で書くとなにがいいの？
  • PHPMD の CyclomaticComplexity（関数の複雑性） の発生を防ぐことができる。

● optional と 三項演算子をうまく使う

下記コードの①②は同じ結果になる。

sample-blade.php

// ①
@if (is_null($comment->created_at))
<span>なし</span>
@else
<span>{{ $comment->created_at->format('Y/m/d') }}</span>
@endif

// ②
// `optional()` と三項演算子でスマートに！！
<span>{{ optional($comment->created_at)->format('Y/m/d') ?? 'なし' }}</span>

• optional ヘルパについて
  • これを使えば、is_null での判定も不要になり、かなり綺麗に書ける
• 三項演算子は if 文で書くには冗長すぎるようなときに使うといいかもしれない。

● @unless が使える

下記コードの①②は同じ結果になる。

sample-blade.php

// ①
@if (!＄user->isFemale())
<p>男だよ</p>
@endif

// ②
// unless が使える
@unless (＄user->isFemale())
<p>男だよ</p>
@endunless

• unless で書けば、! で条件を判定させる必要がないから、よりわかりやすい。

■ 文字列演算子 . の代わりに sprintf を使う

指摘されたくない度★★★☆☆
文字列演算子で結合すると、複雑なものだと読み辛くなる。
sprintf で書くのがいいかもしれない。

$time = '今朝'
$food = 'ホットケーキ'
$num = 2

// 読みづらい
echo $time . '、' . $food . 'を' . $num . '枚食べました。';

// 読みやすく、修正もしやすい
echo sprintf('%s、%sを%d枚食べました。', $time, $food, $num);

■ メソッドに切り出す

指摘されたくない度★★★☆☆

Post クラスは status カラムを持っていて、status が 1 のとき、編集可能だとする。

class Post
{
    //
}

$post = new Post();

if ($post->status === 1) {
    // 処理
}

上記の例では、クラス外でその Post が編集可能かを判断するとき、$post->status === 1 としなければならない。よって、初めてこのコードを読む人は status が 1 とはどんな状態の Post なのか、確認する必要がある。

####[対処法]
Post クラスに isEditable() というメソッドを追加した。
さらに、1 はどのステータスなのか分かりづらいから、EDITABLE 定数に入れた。
これで別クラスで Post が編集可能かを判定するときも、可読性は高く保てる。

class Post
{
    private const EDITABLE = 1;

    public function isEditable(): bool
    {
        return $this->status === self::EDITABLE;
    }
}


$post = new Post();

if ($post->isEditable()) {
    // 処理
}

■ whereKey() whereNotKey() が使える

指摘されたくない度★★★☆☆

whereKey() を使えば、where や whereIn メソッドの第1引数でプライマリーキーの指定が不要になる。

public function getPost()
{
    return Post::where('id', $hoge);
}

// whereKey() でよりシンプルに
public function getPost()
{
    return Post::whereKey($hoge);
}

whereNotKey() は whereKey() の逆。
参考：https://readouble.com/laravel/5.4/ja/upgrade.html

■ $loop を使う

指摘されたくない度★★★☆☆
foreach() でいちばん最初のループだけある処理をしたい、というとき $loop を使うと綺麗に書ける。

@foreach ($posts as $key => $value)
@if ($key === 0)
// 処理
@endif

@endforeach

// $loop を使ってシンプルに
@foreach ($posts as $post)
@if ($loop->first)
// 処理
@endif

@endforeach

$loop のプロパティは他にもある。
参考；Laravel:$loopによるループ変数の使用例

まとめ

プルリクを作り、レビュー依頼をする前に、自分の書いたコードを確認し直す習慣をつけていきましょう。

以下に、どんな言語でも使えそうなチェックリストを置いておくので、コピペして使ってみてください！

- [ ] スペルミスがないか
- [ ] インデントずれがないか
- [ ] 余分なスペースがないか
- [ ] ファイル末尾改行
- [ ] デバッグ、コメントアウトの消し忘れ
- [ ] 使っていないメソッドの消し忘れ
- [ ] 厳密等価演算子 ===
- [ ] メソッド、変数などの命名