フィヨルドブートキャンプというプログラミングスクールで初めてのPRに対するレビューをたくさんやっています。これらの指摘は世界中で行われていると思うので無駄が減るように書きました。

Files changedを確認したか

Image from Gyazo

真っ先に注意したい点。

ローカルの自分のエディタ上で確認していたとしても、PRを作ったら必ずFiles changedを必ず確認する癖をつけよう。GitHubのソースコードビューワーの機能で色々と気づける点もある。

明らかにFiles changedを一回も見てないなというPRはわかるので職場の先輩に怒られる。

lintを通しているか

rubyだったらrubocop、jsだったらeslint

どういうルールが良いかという高度は話題は置いておいて、まずはlintを通しているか否か。

ファイルの末尾に改行が無いか

これ

Image from Gyazo

どのエディターでも設定で自動的に入るようにできるので設定すること。

同じコミットログが続いていないか

適切な文章などの高度な話題以前に明らかに適当になっている場合がある。コミットログというのはちゃんと見られるものだということを理解しよう。

コミットログが英語/日本語になってないか

プロジェクトのルールに合わせましょう。フィヨルドブートキャンプでは統一されていればどちらでもいいです。

コミットログが雑になってないか

modifyだけ。追加だけは後から見た人が困るので駄目。困る人には1ヶ月後の自分も含まれる。

コミットしたユーザーアイコンがOctcatになっていないか

gitクライアントに設定したemailアドレスがGitHubのユーザーのメールアドレスと違う場合にこうなる。

他のチームメンバーからすると「謎のアイコンの人がコミットしてる!」という見た目になる。

これをやると他のメンバーに「ああ、普段コード書いてない(GitHubを使ってない)人なんだな・・・」という生暖かい視線を浴びる羽目になる。

無意味なファイルがないか

generatorが作成した無意味なファイルは削除しよう。実装のないmoduleとか、中身のないtest、fixturesとか。

「これはgeneratorが勝手に作成したんで」というと先輩にすごく怒られる。一つ一つどんな意味を持っているのか調べよう。

gemのgroupが適切か

testでしか使わないgemがgroup無しで全groupで読み込まれている。必要なところだけにしよう。

不要なrespond_toがないか

jsonでアクセスする必要が無いのにscaffoldのコードをそのままで残っている。htmlだけが必要ならrespond_toformatは削除しよう。削除するときにrespond_toformatが何をやっているのか調べた上で消すのも忘れずに。

何でもリファクタリングと言ってないか

リファクタリングはプログラムの振る舞いを変えずに内部構造を変えることなので、ちょっとした修正という意味で使っていると先輩に怒られるというあるある。

秘密の情報がコミットされていないか

oauthのsecretとか。コード中では環境変数にしておこう。

これをやると、gitで過去を修正したとしてもGitHubのキャッシュに残ってしまい、完全に消すにはGitHubのサポートに連絡するとか面倒なことになる。

チームメンバーが「あ〜あ」という顔になる。

.gitignoreは適切か

.DS_Storeはmacだけのファイル。.ideaはJetBrainsのエディター独自のファイル。そういった特定の環境の人向けのファイルは.gitignoreに含めるべきじゃない。

プロジェクト毎の設定じゃなくて自分のマシン固有の設定に書くべき。

大量の依存gemがコミットされていないか

gemのインストール場所をvendro/bundleとかにしてあって、その中にある大量のgemのファイルがコミットされてしまっていないか確認すること。

ネイティブバイナリが含まれたgemもあるので環境によって違うので各自がインストールするものです。

マジックナンバーが無いか

これ。

マジックナンバー (プログラム) - Wikipedia

メソッドが長すぎないか

「1メソッドは五行以内に収めるべし」というSandi Metzルールというものがあります。

そこまで徹底しなくて良いですが、何十行もあると「見づらいな〜、なんとかならない?」ってなります。

綺麗な設計を身に付けるためのSandi Metzルール | A-Listers

CIをパスする前にレビュー依頼をしない

自分は何にも変えてないから通るはずだと過信しない。レビュアーの先輩に怒られる。

5.2系では読み込まれてたんだけど、6.0.1にしたらデフォルトでは読み込まれなくなってるみたい。zeitwerkになったからかしら。

手動でrequireすればOK。

require "active_record/fixtures"

class NotificationMailerPreview < ActionMailer::Preview
  def came_comment
    id = ActiveRecord::FixtureSet.identify(:report_5)

    # ....
  end
end

「開発環境のユーザーのパスワードって何?」

みたいな情報の共有って面倒ですよね。any_login入れちゃうのがいいかも。

any_loginを入れると画面の左下にアイコンが出てきて、そこをクリックするとユーザーが一覧できるプルダウンメニューが出てきます。それを選択するだけでそのユーザーでログインできるというもの。

似た仕組みを用意してる人が多いとは思いますが、無いなら導入おすすめです。

igorkasyanchuk/any_login: Easy way to login as any user in system

認証ライブラリはDevise, Authlogic, Clearance, Sorceryなど色々対応してます。

Image from Gyazo

フィヨルドブートキャンプのアプリに入れてみました。

fjordllc/bootcamp: プログラマー向けEラーニングシステム

フィヨルド宛にwillnetさんからRuby on Rails 6 エンジニア 養成読本をいただきましたー。

Image from Gyazo

rails6になって新しく押さえておかなきゃいけないところをチェックしたかった自分にはちょうど良い本でした。

特にこれらはRailsGuideでもまだWIP状態なので本にまとまってるのはわかりやすかった。

  • Action Text
  • Action MailBox
  • Multiple Database

さらにWebpacker、StimulusについてはREADME・公式サイト以外の情報はあまりないので少しでも記述されている本は集めたい気がします。

testunit(minitest)信者として一番良かったのは、rspecじゃなくてminitestでのテストを一通り説明してあるところ。

これの本は人によって自分が詳しく知ってるところ、弱いところが違うのでありがたい箇所も人それぞれだと思います。(僕はEarly Hints知らなかったので助かった)

ちょっとだけRailsから離れてて復帰した人、Rails6の新機能を押さえたい人にとっては最適な本だと思います。

あえて分けられているのでその認識の上でやるならば、

Rakefileに下記を追加する。

Rake::Task["test"].enhance ["test:system"]

事前にレビューさせていただいた、技術書典6で配布されたRubyとRailsの学習ガイドを@igaiga555さんからいただきました。

Image from Gyazo

チラみせ

Image from Gyazo

これからrubyの勉強を始める人に最適のガイド。

傍らに置いておけば、わからない用語が出てきたときには「これに載ってたやつだ」となるかもしれない。

見て思ったのはむしろRuby・Railsをキーワードに勉強始めてる人の方が役に立つかもしれないということ。「これからどの方向に深めていけばいいのか」がわかりやすいので。

これとRuby超入門を読めばかなりスムーズなスタートがきれるんじゃないでしょうか。

フィヨルドブートキャンプでは新しく来る人に配ろうと思っております。

電子版がBoothで500円なので気になる人は何も考えず買っちゃっていいのでは?

関連

idの順番に依存しないコードを書こう - komagataのブログ

このエントリーに @jnchito さんがエントリーを書かれていました。

【Rails】idでソートするか?created_atでソートするか? 〜 Re: idの順番に依存しないコードを書こう - Qiita

こちらに対する僕の考えも書いておこうと思います。(こういうのってブログっぽいですね!)

  1. created_atでソートするときはcreated_atにインデックスを貼る。
  2. 意図が無い場合はソート自体しない。

1はそのままですね。created_atでソートしない場合は貼らなくて良い。元エントリーもそれ前提で書いてました。

2は「idでソートしたい」という意図がない場合「とりあえず何らかのソートをしなくちゃ」と思ってる場合はソート自体しなくて良い。ランダムなuuidでソートするのとソートしないのと変わらんと思う。

ただ、ソーシャルゲームとかその他の超膨大なデータを処理する時にパフォーマンスの問題でこの前提をあえて崩すのは良いと思う。非正規化と似たようなもん。ただあくまで基本はidでソートしない方が良いということ。

最近レビューでよく書くのでまとめておきます。

指摘

idでソートするとか、idがインクリメンタルな数字ということを前提とするコードは避けよう。古い順に並べたいんだったらcreated_atとかが作成日なんだから「古い順」ということをより表現していることになる。

理由

idをuuidとかに変えた場合バグになる。

古いものを取ろうとしてidでソートしてfirstとかやっちゃいかん。

管理画面のユーザー一覧をid降順でソートしてて、最近追加されたユーザーは一番最初にきてたのuuidにしたら並びが変わった・困るとお客さんに言われることになる。

rails+postgresでidにuuidを使う - komagataのブログ

Microsoft系のライブラリやフレームワークもuuidがidのこと多い。

railsもfixturesではidはランダムに入るのでidがインクリメンタルな数字という前提のテストはコケる。(偶然動いたりするがそのうちコケる)

補足

もちろんだけど、こういうコードは全く問題ないヨ!

@post = Post.find(params[:id])

むしろidはidentifierなんだから本来の使い方。気をつけるのはソート。

つづき

created_atでソートする場合はインデックスを貼る - komagataのブログ

ブートキャンプのアプリをDEPRECATEDなpaperclipからactive_storageに移行した。

GCSのcredential関係ってPrivate keyがそのまま入ってて改行のせいでうまく行かなかったりとかいつもハマる。エラーメッセージの内容も原因解明にあまり役に立たなくて結局ガッツリデバッグすることになって時間がかかる。

ActiveStorage + GCS + Herokuでの自分的ポイントはGCSからダウンロードできるJSONをそのまま環境変数に入れること。

storage.ymlはこんな風にするのがよい。

config/storage.yml:

google:
  service: GCS
  project: "bootcamp-224405"
  credentials: <%= ENV["GOOGLE_CREDENTIALS"] %>
  bucket: "bootcamp-fjord-jp"

こういう感じで入れておく。

$ heroku config:set GOOGLE_CREDENTIALS="$(< /path/to/bootcamp.json)"

実際にちゃんとconfigの各値が読めてるかどうかはActiveStorage::Service#configあたりをデバッグすればよい。

フィヨルドブートキャンプでも草が生えるようにしました。

Image from Gyazo

最初は色も全く同じだったんですが、実装したことに満足して冷静になってみると、

「Githubと同じ色だと紛らわしいな」

とか

「Gtihubと違って一年も勉強してたらやばいから3ヶ月表示ぐらいがいいな」

などあり、もうちょっと変更していきたいと思います。

超実践的プログラミングスクール フィヨルドブートキャンプ