フィヨルドブートキャンプというプログラミングスクールで初めてのPRに対するレビューをたくさんやっています。これらの指摘は世界中で行われていると思うので無駄が減るように書きました。
Files changedを確認したか
真っ先に注意したい点。
ローカルの自分のエディタ上で確認していたとしても、PRを作ったら必ずFiles changedを確認する癖をつけよう。GitHubのソースコードビューワーの機能で色々と気づける点もある。
明らかにFiles changedを一回も見てないなというPRはレビュワーもそれと分かります。
lintを通しているか
rubyだったらrubocop、jsだったらeslint
どういうルールが良いかという高度は話題は置いておいて、まずはlintを通しているか否か。
ファイルの末尾に改行があるか
これ
どのエディターでも設定で自動的に入るようにできるので設定すること。
同じコミットログが続いていないか
適切な文章などの高度な話題以前に明らかに適当になっている場合がある。コミットログというのはちゃんと見られるものだということを理解しよう。
コミットログが英語／日本語になってないか
プロジェクトのルールに合わせましょう。フィヨルドブートキャンプでは統一されていればどちらでもいいです。
コミットログが雑になってないか
modifyだけ、追加だけは後から見た人が困るので駄目。困る人には1ヶ月後の自分も含まれる。
コミットしたユーザーアイコンがOctcatになっていないか
gitクライアントに設定したemailアドレスがGitHubのユーザーのメールアドレスと違う場合にこうなる。
他のチームメンバーからすると「謎のアイコンの人がコミットしてる！」という見た目になる。
これをやると他のメンバーに「ああ、普段コード書いてない（GitHubを使ってない）人なんだな・・・」という生暖かい視線を浴びる羽目になる。
不要なファイルをコミットしてないか
.DS_Store（Macのみ）とかxxx~（viのみ）とかその人にしか必要ないファイルを含めてしまってないか。開発に参加するみんなに必要なものだけを含めるようにしよう。
無意味なファイルがないか
generatorが作成した無意味なファイルは削除しよう。実装のないmoduleとか、中身のないtest、fixturesとか。
「これはgeneratorが勝手に作成したんで」
といってファイルがカオスになってるプロジェクトは誰も触りたがらない。
一つ一つどんな意味を持っているのか調べよう。
gemのgroupが適切か
testでしか使わないgemがgroup無しで全groupで読み込まれている。必要なところだけにしよう。
不要なrespond_toがないか
jsonでアクセスする必要が無いのにscaffoldのコードをそのままで残っている。htmlだけが必要ならrespond_toやformatは削除しよう。削除するときにrespond_toとformatが何をやっているのか調べた上で消すのも忘れずに。
何でもリファクタリングと言ってないか
リファクタリングはプログラムの振る舞いを変えずに内部構造を変えることなので、ちょっとした修正という意味で使ってはいけない。
秘密の情報がコミットされていないか
oauthのsecretとか。コード中では環境変数にしておこう。
これをやると、gitで過去を修正したとしてもGitHubのキャッシュに残ってしまい、完全に消すにはGitHubのサポートに連絡するとか面倒なことになる。
チームメンバーが「やっちまったな・・・」という顔になる。
.gitignoreは適切か
.DS_Storeはmacだけのファイル。.ideaはJetBrainsのエディター独自のファイル。そういった特定の環境の人向けのファイルは.gitignoreに含めるべきじゃない。
プロジェクト毎の設定じゃなくて自分のマシン固有の設定に書くべき。
大量の依存gemがコミットされていないか
gemのインストール場所をvendro/bundleとかにしてあって、その中にある大量のgemのファイルがコミットされてしまっていないか確認すること。
ネイティブバイナリが含まれたgemもあるので環境によって違うので各自がインストールするものです。
マジックナンバーが無いか
これ。
メソッドが長すぎないか
「1メソッドは五行以内に収めるべし」というSandi Metzルールというものがあります。
そこまで徹底しなくて良いですが、何十行もあるとさすがに見辛いので修正が必要です。
綺麗な設計を身に付けるためのSandi Metzルール | A-Listers
CIをパスする前にレビュー依頼をしない
自分は何にも変えてないから通るはずだと過信しない。
理解せずコピペしてないか
ググった内容を自分のコードに取り入れることは問題ありません。しかし、その内容を自分で理解せずそのまま使うのはよくないです。理解していないコードでは何が起きるか分かりません。
日本語の作文をするのに似た内容だからといって他の本からそのまま取ってきたら違和感のある文章になってしまうのと同じように、プログラムも変数名や構造を自分のプログラムに合わせずにそのまま使えることはほぼありません。理解しているプログラマーにとっては明らかに違和感のあり無駄のあるコードに見えます。
なので、
「自分の書いたこの部分が何をやってるか理解してる？」
と先輩に聞かれると言うことがよくあります。
意味を理解した上で自分のコードの文脈に合わせた内容に変えて使いましょう。