こんばんは、きょとん(@ippomihosanpo)です。
体調悪いし、そもそも大人数は苦手だし、イベントに出不精になっていましたが、久々に勉強会に参加してきました。

TODOアプリ作ってレビュー会しよう！という企画の、アプリをつくるもくもく会の回でした。

わたしのスペック

JavaEE(専門2年) => JavaEE(仕事4年) => PHP(仕事2年) => Ruby(仕事2年) くらい
Rails4系のプロダクトの経験がほとんど
Rails5系の仕様理解度はまだ40%くらい(がんばろう

参加動機

Rails有識者にレビューしてもらったことがないからみてほしいな
いま一人で新規開発してるけど、変な作り方してないか不安すぎるんだよな
時間があるときにできるだけアプリ作っておきたいな、ほら、ニートだし
だんだん元気になってきたから、外に出て人と会おうかな(がんばろう

レビューしてもらったもの

もくもくの回だったのですが、私がフライングでざっくりとしたアプリを作ってきたため、レビューしてもらいました。（作業の手止めて本当にすみませんでした…）
※ 未完成・レビュー内容未反映です(8/31現在)

つくってきたもの
ExpertTodo
テストアカウント：test@test.com / testtest
レビュー内容
GitHub
指摘してもらったメモと、後から見直してここ直さなきゃなと思ったメモがごちゃまぜになってます。

レビューまとめ

上記で指摘してもらったことをもとに自分でまとめてみました。
間違えていたり、用語の使い方がおかしかったら言ってください。

冗長な書き方しない

if文の条件判定がむだなことしてたり、関連を上手く使わずにデータを取得していたところがありました。
xxx.present? で、各所で三項演算子祭りしてたのが、presenceでかなりスマートになると知り目から鱗でした。

# 三項演算子
params[:referer].present? ? params[:referer] : '/'

# presenceってすぎょい
params[:referrer].presence || '/'

Railsが用意してくれてるのに使えてない便利なメソッド他にもある気がします…。

「?」がつくメソッドはbooleanを返すことを期待する

※8/31追記
ログイン判定が冗長なのもありますが
「logged_in?」というメソッド名だけ見ると既にログインしているかどうかチェックしbooleanを返すメソッドのように見えるのに、実際にはログインしてない場合リダイレクト処理まで行い、booleanも返しません。
わかりづらいですね。
メソッド名変えた方がよさそうです(ついでに冗長なところもなおしてみます)。

application_controller.rb

# ログインチェック
def logged_in?
  user_id = session[:user]
  if user_id then
    begin
      @current_user = User.find(user_id)
    rescue ActiveRecord::RecordNotFound
      # 存在しないユーザであればログアウトする
    end
  end
  unless @current_user
    # ログインしてなければ、ログイン画面に飛ばす
    # ログイン後、リクエストされたページに戻れるようにリファラを渡しとく
  end
end

application_controller.rb

# ログインチェック
def check_logged_in
  @current_user = User.find_by(id: session[:user])
  # ログインしてない場合、またはセッションに登録されているIDに該当するユーザがいない場合
  unless @current_user
    # ログアウト
    # ログイン画面に飛ばす
    # ログイン後、リクエストされたページに戻れるようにリファラを渡しとく
  end
end

コントローラは薄く

持ち前のFatControllerおばさん気質がもろに出てしまいました。
(※8/31追記言い訳させてください=>なぜFatControllerおばさんしてしまうのか)
モデルでやるべき仕事をコントローラでやらない。
下記で書く「抽象化、下位層のロジックの隠蔽」にも関連する気がします。

コントローラはCRUDのみを使用した方が良い

これは全く知りませんでした…。
たとえばタスクを完了する処理の場合はたぶんこんな感じです。

tasks_controller.rb

class TasksController < ApplicationController
  def finish
    # タスク完了する処理
  end
end

finish_controller.rb

class Tasks::FinishController < ApplicationController
  def update
    # タスク完了する処理
  end
end

あとから見てこれが大変参考になりました。
DHHはどのようにRailsのコントローラを書くのか

わかりやすい名前で、開発者に優しく

ログイン認証するところを自作してLoginControllerと命名したのですが、gemのDeviceを使った場合だとSessionsControllerに値するのでそちらでつけた方が開発者にはやさしいとのことでした。
確かにです。自作したので個性を出したくなってしまいました。。

抽象化、下位層のロジックの隠蔽

（※8/31追記コントローラにとってモデルが下位層と言っていいか微妙ですが)
コントローラが呼び出すのは抽象的な処理である必要があるのかなと思いました。
ActiveRecordでDBからデータをごにょごにょするようなロジックなどはモデルに任せて、コントローラはそのモデルのメソッドを呼び出すだけ…がよさそうでした。
具体的に書けば、タスクを完了させる処理はコントローラでこんなかんじでしょうか。

finish_controller.rb

def update
  # コントローラがタスクを完了させるための具体的な処理を知っている
  @task.update(status: 2, finished_at: Time.zone.today)
end

finish_controller.rb

def update
  # 抽象的にする
  @task.finish(Time.zone.today)
end

さいごに

とても有意義な時間でした。レビューしてくれた先輩方に感謝の気持ちでいっぱいです。
やっぱり有識者にレビューしてもらうと気づきが多すぎます。
いいコードかけるようにがんばります。

ぺーぺーの私がRailsのコードをはじめて上級者にレビューしてもらった

わたしのスペック

参加動機

レビューしてもらったもの

レビューまとめ

冗長な書き方しない

「?」がつくメソッドはbooleanを返すことを期待する

コントローラは薄く

コントローラはCRUDのみを使用した方が良い

わかりやすい名前で、開発者に優しく

抽象化、下位層のロジックの隠蔽

さいごに