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