RESTfulにするならデフォルトのルーティングを使わない
元記事はこちら
感想
内容がRails2の時代のものでRails5と書き方が大きく異なるので、翻訳は割愛。 RESTfulなルーティングを採用する場合は、余計なルーティングが生成されると、セキュリティリスクにもなるからやめましょうという主旨だと思う。
resources :posts, :member => { :push => :post }
みたいな書き方をするのは7種類のアクション(index、show、new、edit、create、update、destroy)全部を使う時だけにして、 一つでも欠ける時は個別に書くのがよい。と思う。
コーディング規約が浸透しない問題
コードレビュー、プルリクのタイミングなどでは、レビュアーの指摘は立ち位置をそのプルリク作者の思考地点に揃えるのが私は良いと思う。その前提が作れなさそうなら、実装方針を事前に揉むってことが必要だと思う。そういう流れが作れると割とスムーズになる。
— Hidenori Goto (@hidenorigoto) 2017年5月14日
2017/5/15 20:08 追記
システム開発で、レビューする側もレビューされる側も、成果物をレビューするという意識になっていると不幸になる。作業途中で方向性が間違っていないか?をレビューする
— てるろー (@terurou) 2017年5月14日
コーディング規約をつくっても読まれなかったり、定着しなかったりで、うまくいった試しがなくて、 なんでかなーと思っていたけれど、このツイートをみて、「実装方針の共有」ってところがキモになるんじゃないかと思った。
今まで見てきた/作ってきたコーディング規約だと、コーディングスタイルの話と、実装方針の話がごっちゃになって箇条書きにされているだけというのが多かった気がする。 コーディングスタイルを統一させたいのであれば、PHPならphpcsとか、rubyならrubcopとかあるし、Scrutinizerとか、SideCIでも静的解析はしてくれるので、そういうものを人間が目で確認しなくてもいいように取り入れていくべき。
実装方針については、フレームワークやライブラリによってある程度縛られる部分はあるし、エンジニアなんだから使っている技術は十分に知っとけっていう人もいるかもしれないけれど、スポットで入る人とか、初めて触る人との共有はやはり難しいと思うので、「こういう処理が書きたくなったら、このコンポーネントにまとめてね。」とか「この部分はテストパターンも厚めに書いてね」とか、共有できると、PR見る方も、投げる方も幸せになれるような気がする。
OSSでなくてもCONTRIBUTING.md作ってあらかじめ共有しときたいことは書いておいたほうが良いのだろうか。それでもやっぱり読まない人は読まないから、無駄なんだろうか。 「これくらい知っとけ」の場合でも、「この記事は読むべし」みたいな一覧があると、ミスコミュニケーションが原因でPR全部書き直しみたいな事態は防げそうだけど、大変でも苦労して頑張れ事案なのだろうか。
入れ子モデルフォーム
元記事はこちら
翻訳
Bad Smell
class Product < ActiveRecord::Base has_one :detail end class Detail < ActiveRecord::Base belongs_to :product end <% form_for :product do |f| %> <%= f.text_field :title %> <% fields_for :detail do |detail| %> <%= detail.text_field :manufacturer %> <% end %> <% end %> class ProductsController < ApplicationController def create @product = Product.new(params[:product]) @detail = Detail.new(params[:detail]) Product.transaction do @product.save! @detail.product = @product @detail.save end end end
ProductモデルとDetailモデルには1対1の関係があり、productと一緒にmanufacturerも保存したいとします。この場合、productオブジェクトとdetailオブジェクトを生成し、トランザクションによって関連付けます。でも、なんでこんな込み入った処理をコントローラーでやっているのでしょう。Productモデルがdetailの生成を担当すべきです。
Refactor
class Product < ActiveRecord::Base has_one :detail accepts_nested_attributes_for :detail end <% form_for :product do |f| %> <%= f.text_field :title %> <% f.fields_for :detail do |detail| %> <%= detail.text_field :manufacturer %> <% end %> <% end %> class ProductsController < ApplicationController def create @product = Product.new(params[:product]) @product.save end end
Productモデルに、accepts_nested_attributes_forを追加したので、ProductモデルはDetailモデルの生成を制御できるようになりました。これは良いことです!
accepts_nested_attributes_forのおかげで、1対多の関係における生成も簡単にできるようになります。
class Project < ActiveRecord::Base has_many :tasks accepts_nested_attributes_for :tasks end class Task < ActiveRecord::Base belongs_to :project end <% form_for @project do |f| %> <%= f.text_field :name %> <% f.fields_for :tasks do |tasks_form| %> <%= tasks_form.text_field :name %> <% end %> <% end %>
railsに素晴らしいメソッドが用意されていて、実にありがたいことです!
感想
Railsガイドの関連付けに関する章を最近読みながら書いてますが、いまいち理解できていない。 関連付けの仕方はわかってきたものの、生成のさせ方と検索時に取れるレコードの形(Assosiation/Array)の判別がいまいち腑に落ちていないというか、モヤっとしている。 今日もちょうどBadSmellなコードを書いてしまったので、テスト書いてリファクタリングせねば。。
無駄に深いネスト
元記事はこちら
翻訳
Bad Smell
map.resources :posts do |post| post.resources :comments do |comment| comment.resources :favorites end end <%= link_to post_comment_favorite_path(@post, @comment, @favorite) %>
3階層にネストしたルーティングは本当に必要でしょうか?ルーティングをネストするということは、子階層のリソースが親階層のリソースに依存しているということを意味します。 上記の例では、commentsというリソースがpostに依存しているということです。 しかし、favoritesはcommentに依存するものですから、favoritesのリソースがpostに依存している必要は全くありません。
Refactor
map.resources :post do |post| post.resources :comments end map.resources :comments do |comment| comment.resources :favorites end <%= link_to comment_favorite_path(@comment, @favorite) %>
以上の理由から、3階層のネストを2階層に減らしたルーティングにリファクタリングしました。favoritesへのurlもこうすることで短くなります。
Updated by @flyerhzm
無駄に深いネストを回避する新しい方法です。
Refactor 2
map.resources :posts, :shallow => true do |post| post.resources :comments do |comment| comment.resources :favorites end end <%= link_to comment_favorite_path(@comment, @favorite) %>
見ての通り、shallow => true
オプションをpostのルーティングに追加することで、1階層か2階層以内に抑えてルーティングを生成します。
以下のように生成されます。
posts GET /posts(.:format) {:action=>"index", :controller=>"posts"} POST /posts(.:format) {:action=>"create", :controller=>"posts"} new_post GET /posts/new(.:format) {:action=>"new", :controller=>"posts"} edit_post GET /posts/:id/edit(.:format) {:action=>"edit", :controller=>"posts"} post GET /posts/:id(.:format) {:action=>"show", :controller=>"posts"} PUT /posts/:id(.:format) {:action=>"update", :controller=>"posts"} DELETE /posts/:id(.:format) {:action=>"destroy", :controller=>"posts"} post_comments GET /posts/:post_id/comments(.:format) {:action=>"index", :controller=>"comments"} POST /posts/:post_id/comments(.:format) {:action=>"create", :controller=>"comments"} new_post_comment GET /posts/:post_id/comments/new(.:format) {:action=>"new", :controller=>"comments"} edit_comment GET /comments/:id/edit(.:format) {:action=>"edit", :controller=>"comments"} comment GET /comments/:id(.:format) {:action=>"show", :controller=>"comments"} PUT /comments/:id(.:format) {:action=>"update", :controller=>"comments"} DELETE /comments/:id(.:format) {:action=>"destroy", :controller=>"comments"} comment_favorites GET /comments/:comment_id/favorites(.:format) {:action=>"index", :controller=>"favorites"} POST /comments/:comment_id/favorites(.:format) {:action=>"create", :controller=>"favorites"} new_comment_favorite GET /comments/:comment_id/favorites/new(.:format) {:action=>"new", :controller=>"favorites"} edit_favorite GET /favorites/:id/edit(.:format) {:action=>"edit", :controller=>"favorites"} favorite GET /favorites/:id(.:format) {:action=>"show", :controller=>"favorites"} PUT /favorites/:id(.:format) {:action=>"update", :controller=>"favorites"} DELETE /favorites/:id(.:format) {:action=>"destroy", :controller=>"favorites"}
:shallowは良い解決策だと思います。このオプションにより、以下のような使い方ができます。
post_comment_path(1, 1) comment_path(1) comment_favorite_path(1, 1) favorite_path(1)
素晴らしい!でもrailsドキュメントに載っていないのが残念です。
感想
railsドキュメントに載っていない 掲載されるようになったみたい。 ActionDispatch::Routing::Mapper::Resources
これも読んで、できる限りRESTfulなルーティングに近づけるようにしていると、1アクションの責務がとても明確になって良い。 postd.cc
model.collection_model_ids (多対多)
元記事はこちら
翻訳
Bad Smell
class User < ActiveRecord::Base has_many :user_role_relationships has_many :roles, :through => :user_role_relationships end class UserRoleRelationship < ActiveRecord::Base belongs_to :user belongs_to :role end class Role < ActiveRecord::Base end <% form_for @user do |f| %> <%= f.text_field :email %> <% Role.all.each |role| %> <%= check_box_tag 'role_id[]', role.id, @user.roles.include?(role) %> <%= role.name %> <% end %> <% end %> class UsersController < ApplicationController def update @user = User.find(params[:id]) if @user.update_attributes(params[:user]) @user.roles.delete_all (params[:role_id] || []).each { |i| @user.roles << Role.find(i) } end end end
見ての通り、UserモデルとRoleモデルが多対多の関係で、1ユーザーに複数のroleを設定できるフォームがあるとします。 コントローラーではまず、ユーザーの権限を全て削除してから、ユーザーがチェックボックスで選択した権限を割り当てています。 自前で権限の割り当てをするのは見苦しいので、railsの仕組みにのるべきです。
Refactor
<% form_for @user do |f| %> <% Role.all.each |role| %> <%= check_box_tag 'user[role_ids][]', role.id, @user.roles.include?(role) %> <%= role.name %> <% end %> <%= hidden_field_tag 'user[role_ids][]', '' %> <% end %> class UsersController < ApplicationController def update @user = User.find(params[:id]) @user.update_attributes(params[:user]) # the same as @user.role_ids = params[:user][:role_ids] end end
チェックボックスの名前を"role_id“ から "user[role_ids]"に変更しました。こうすることで、role_idsは自動的にUserモデルに割り当てられます。 hidden_field_tag に'user[role_ids][]‘があることに注意してください。値は空で、ユーザー権限のチェックボックスが選択されなかった場合、権限が全て削除されることを保証します。 これでコントローラーが簡潔になりました。
感想
Rails4~だとStrongParams使うので、コントローラーは以下のようになるはず。
class HomeController < ApplicationController def update p params @user = User.find(params[:id]) @user.update_attributes(role_params) # the same as @user.role_ids = params[:user][:role_ids] end private def role_params params.require(:user).permit(:role_ids) end end