こんばんは、きょとん(@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も返しません。
わかりづらいですね。
メソッド名変えた方がよさそうです(ついでに冗長なところもなおしてみます)。
# ログインチェック
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
# ログインチェック
def check_logged_in
@current_user = User.find_by(id: session[:user])
# ログインしてない場合、またはセッションに登録されているIDに該当するユーザがいない場合
unless @current_user
# ログアウト
# ログイン画面に飛ばす
# ログイン後、リクエストされたページに戻れるようにリファラを渡しとく
end
end
##コントローラは薄く
持ち前のFatControllerおばさん気質がもろに出てしまいました。
(※8/31追記 言い訳させてください=>なぜFatControllerおばさんしてしまうのか)
モデルでやるべき仕事をコントローラでやらない。
下記で書く「抽象化、下位層のロジックの隠蔽」にも関連する気がします。
##コントローラはCRUDのみを使用した方が良い
これは全く知りませんでした…。
たとえばタスクを完了する処理の場合はたぶんこんな感じです。
class TasksController < ApplicationController
def finish
# タスク完了する処理
end
end
class Tasks::FinishController < ApplicationController
def update
# タスク完了する処理
end
end
あとから見てこれが大変参考になりました。
DHHはどのようにRailsのコントローラを書くのか
##わかりやすい名前で、開発者に優しく
ログイン認証するところを自作してLoginControllerと命名したのですが、gemのDeviceを使った場合だとSessionsControllerに値するのでそちらでつけた方が開発者にはやさしいとのことでした。
確かにです。自作したので個性を出したくなってしまいました。。
##抽象化、下位層のロジックの隠蔽
（※8/31追記 コントローラにとってモデルが下位層と言っていいか微妙ですが)
コントローラが呼び出すのは抽象的な処理である必要があるのかなと思いました。
ActiveRecordでDBからデータをごにょごにょするようなロジックなどはモデルに任せて、コントローラはそのモデルのメソッドを呼び出すだけ…がよさそうでした。
具体的に書けば、タスクを完了させる処理はコントローラでこんなかんじでしょうか。
def update
# コントローラがタスクを完了させるための具体的な処理を知っている
@task.update(status: 2, finished_at: Time.zone.today)
end
def update
# 抽象的にする
@task.finish(Time.zone.today)
end
#さいごに
とても有意義な時間でした。レビューしてくれた先輩方に感謝の気持ちでいっぱいです。
やっぱり有識者にレビューしてもらうと気づきが多すぎます。
いいコードかけるようにがんばります。