プロのRails開発者に学ぶ！コードレビュー集

　私は現在Happiness Chainというプログラミングスクールに在籍しており、Rails課題でECサイト、Twitterクローンを実装しました。その中でメンターさんから指摘された事をまとめました。間違いがありましたら、ご指摘いただけると嬉しいです。

🙆RESTfulな設計にしましょう

　これはitemsコントローラのindexアクションに割り当てられています。beforeとafterどちらとも生成されるpathはitems_pathです。

修正前

models/routes.rb

  get "/items", to:"items#index"

　リソースベースのルーティングresourcesを使う事でCRUDアクションのルーティングを一つ一つ記述しなくても、HTTPメソッドの(GET、POST、PUT、PATCH、DELETE)がそれぞれ、コントローラの(index、show、new、edit、create、update、destroy)というアクションに対応して自動的に生成されます。これによりRESTfulな原則に基づいて設計できます。

sample.rb

 resources, :items

#ターミナル
$rails routes

    items GET /items(.:format)          items#index 
          POST /items(.:format)         items#create 
 new_item GET /items/new(.:format)      items#new 
edit_item GET /items/:id/edit(.:format) items#edit 
     item GET /items/:id(.:format)      items#show 
          PATCH /items/:id(.:format)    items#update 
          PUT /items/:id(.:format)      items#update 
          DELETE /items/:id(.:format)   items#destroy

　特定のアクションだけのルーティングを生成したい場合、:only オプションを使用して指定することもできます

修正後

models/routes.rb

resources, :items, only: %i[index]　 

🙆アソシエーションを使いましょう

　deviseならcurrent_userでRailsアプリケーションで現在ログインしているユーザーを取得できる。全てのツイートから自分のツイートを取得しようとしている。

修正前

controllers.sample.ruby

 @my_tweets = Tweet.where(user_id: current_user.id)

　Userとツイートは以下のように関連づけているので

app/models/user.rb

 class User < ApplicationRecord
  has_many :tweets, dependent: :destroy
 end

app/models/tweet.rb

class Tweet < ApplicationRecord
  belongs_to :user
end

　current_user.tweetsのようにアソシエーションを使って自身の全てのツイートを取得できます。

修正後

ontrollers.sample.ruby

 @my_tweets = current_user.tweets

🙆DB制約は厳重にしましょう

可能な限りnull falseをつけます。

  def change
    create_table :users do |t|
      t.string :name, null: false
      t.string :email, null: false

　:user, :email 属性が存在し（nil や空文字列でない）、データが保存される際に必須であることを指定しています。

app/models/user.rb

class User < ApplicationRecord
  validates :user, presence: true
  validates :email, presence: true
end

ただ、このバリデーションは単に値が存在するかどうかのみを確認しているので、例えば「12ekpddke」のような実際にはemailとして不適切な値でも保存されてしまいます。そのため、正規表現を用いて期待するデータ形式をより厳密に指定します。他にも電話番号、郵便番号、WebサイトのURL、テキストの長さといった値なども厳密に指定する必要があります。生年月日の欄で未来の日付だったり、西暦100年などありえない数値のときもエラーとなるように制約ができます。

app/models/user.rb

class User < ApplicationRecord
  validates :user, presence: true
  validates :email, presence: true, format: { with: /\A[\w+\-.]+@[a-z\d\-.]+\.[a-z]+\z/i/ }
end

🙆空文字で送信されたときはValidationエラーを表示するといいでしょう

tweets_controller.rb

 def create
    @tweet = current_user.tweets.build(tweet_params)
    if @tweet.save
      redirect_to root_path
      flash[:success] = 'ツイートしました'
    else
   #ステータスコードをバリデーションエラーの場合に返すシンボル表現にする
      render 'index', status: :unprocessable_entity
    end
  end

models/tweet.rb

class Tweet < ApplicationRecord
  validates :text, presence: true
end

tweets/_form.html.slim

  = form_with model: tweet do |f|
/バリデーションエラーメッセージは他のフォームでも使うので共通化するといい
    = render 'layouts/error_messages', model: tweet
/以下省略

🙆エラーメッセージにstyleを当てると尚いいでしょう。私はエラーメッセージは赤色にしました。

layouts/_error_messages.html.slim

- if model.errors.any?
  div class="alert alert-danger"
    ul
      - model.errors.full_messages.each do |message|
        li = message

　Validationエラーメッセージは日本語に変更できます。

意識するようになったこと

　Rails scaffoldのindex show edit create update destroy がシンプルな構成となっているのが理想であることを知りました。ですのでコントローラはあれやこれや情報を入れないで限りなくシンプルになるように意識しました。また複雑なロジックは全てモデルへ移行するようになりました。

開発する上で参考にしたサイト