プログラミング 美徳の不幸

Ruby, Rails, JavaScriptなどのプログラミングまとめ、解説、備忘録。

Rails初心者に知ってほしい本当の脱初心者の方法

qiita.com

この記事読みました。まぁ感想を一言で言えば

http://nplll.com/assets/2011/02/13_01.jpg

まぁ言いたいことを順を追って言っていきます。

ロジックで用いる具体的な数字は定数に格納する

これは半分正しいですが半分間違いです。マジックナンバー死すべしみたいな人が短絡的にあらゆる数値をそのクラスの定数にしちゃったりしてますが、程度問題です。

例えば

@articles = Article.where(...).page(params[:page]).per(PAGINATE_ITEM_LENGTH)

みたいなやつ。こういうの、結局ここでしか使わないから修正箇所1個だけなんですよね。だからベタ書きでもいいと思います。 定数にしたほうがいいやつってむしろこういうやつで

$li_margin_top : 50px;
ul {
  margin-top: -$li_margin_top;
}

li {
  margin-top: $li_margin_top:
}

まぁ変更する頻度が高い、どこかの数字と連動している、とか。

CRUDとルーティング

ある時期まではとりあえずresourcesが使えないか考えるべしというのは姿勢としてはいいのですが、現実問題としてわりと厳しいです。 例えばあるレストランの詳細ページの中に、地図を表示するページとかメニューを表示するページとかいろいろ作る時

resources :restaurants, only: %w[show] do
  resource :maps, only: %w[show]
  resource :menus, only: %w[index]
  resource :reviews, only: %w[index]
end

みたいな。まぁreviewsとかはいいんですけどmapみたいに明らかに1画面しか作らないなら

resources :restaurants, only: %w[show]
get 'restaurants/:id/map', as: :restaurant_map・・・①

resources :restaurants, only: %w[show] do
  get 'map' => 'restaurants#map', as: :restaurant_map・・・②
end

で十分ですよね。とくに①の方法は:idの部分を:nameとか:uuidとか変えられるのでオススメです。その際named_routesがそれっぽくなるようにちゃんとasオプションを付けましょう。

ちなみにshallowオプションは使うべきではない派です。

resources :restaurants, only: %w[show], shallow: true do
  resources :reviews, only: %w[show]
  #これでレストランのレビュー一覧ページはマッピングできるけど、ユーザのレビュー一覧は?
  # それならRestaurant#ReviewsControllerとUser#ReviewsControllerにしたほうが良くない?
end

まぁas, namespace, pathあたりのオプションをちゃんと使えば良い話です。

クラスを継承してsuperメソッドを使う...

まずDevise使うのやめましょう。 あと、iOS使ってるとsuper.viewDidLoad()とか出てくるけどRailsでこれが出てきたら使うライブラリが間違ってるか設計がおかしいかのどちらかだと思う。

クラスをまたぐ処理はconcernに

concerns使うシーンがピンとこなかったんですが、モデルからデコレータに変換するメソッドだけを含むモジュールとかをconcernsにするとけっこう便利です。あとはpublished_atだけでModel#draft?, Model#published?, Model.published, Model#publish!などを付け足してくれるモジュールとか。

ただ、たまたま共通してるレベルのものをconcernsにするのは危険かなと思います。

scope

昔保守した案件で @article.visible.user_authenticated.high_quolity.(以下略)みたいに定義して、結局Article.publishedが抜けてたみたいな事故があり、案外scope定義しまくるのはリスクがあるというか、scopeがそんなに大量になる時点で何かがおかしい気がします。 後はアプリケーションの画面に強く関係するビジネスロジックをscopeにすべきではないです。

Helperメソッド

ページ数によってtitleタグを動的にとる程度であれば

- if @article.current_page > 1
  title "記事一覧 #{@article.current_page} ページ"
- else
  title "記事一覧"

とかで十分。どうしても欲しいなら@articleのデコレータ。これをヘルパーにしてるとメソッド名が枯渇する。

考え方として、Railsが提供してくれるlink_toとかの亜種を作りたいときにはいいと思う。

  def noindex_nofollow_tag
    "<meta content='noindex,nofollow' name='robots' />".html_safe
  end

とか。

Test

minitest使ってるのはいいですね!

おわりに

いろんな現場、いろんなプロジェクト、規模やセキュリティの重要性、リリース後の動きとかによって書くべきコードは変わると思います。 私もまだまだ勉強中です。

批判あればご指摘ください