新人がよくやる間違い(MVCの領域侵犯編) #Ruby

Railsを使ったアプリケーション制作を始めて2ヶ月ちょっと...。

こんな↓コードを書いていたら、先輩から「それはやばいよ」と指摘を受けました。

<% @users.each do |user| %>
	user_type = UserType.find(user.type_id)
	<div>
	<p><%= user %></p>
	<p><%= user_type.type_name %></p>
	</div>
<% end %>

なにがやばいのかわかりますか?

まあ、タイトルでネタバレしているのですぐわかってしまうと思うんですけど、ViewでSQL文を書いているんですね。端的に云うと、「それコントローラーでやれよ」ってことです。(さら言うとN+1問題も発生しているんですが、今回は割愛)

サンプルではリレーションのあるわかりやすい2つのモデルなので、コントローラーでらくらく書けるんですけど、

例えば直接的なリレーションのない2つのモデルを扱わないといけなかったり...

複雑な関係をもつ3~4つのモデルを同時に扱わないといけなかったり...

ぱっとSQLが書けないときに、「なんか違うな...」と思いつつも、「eachのループ中にSQL書いちゃえ」と安易な方法をとってしまっていました。

他にも、、、

def index
	@current_date = Date.today.strftime("%Y年%-m月%-d日")
end

→コントローラーで、今の日付の表示のさせかたまで書いてしまっていたり...

def index
	@all_users_names = User.all.pluck(:user_name).join('さん、') + 'さん'
end

→名前を連結させて、さん付けさせるところまで書いてしまっていたり...

class User < ActiveRecord::Base

	scope :all_users_names, -> {.all.pluck(:user_name).join('さん、')}

end

→スコープなのにカラムを配列化してさらにjoinまでしてる...

・コントローラーでやるべきこと(=モデルとビューの橋渡し)をビューやモデルでやってしまっている

・ビューでやるべきこと(コントローラーから渡されたデータの表示)をコントローラーでやってしまっている

ということをやっちゃってました...。より抽象的に云うなら、MVCそれぞれの責任領域を逸脱したコードを存在させてしまっていたんですね。

先輩からは、なんでやばいのかを聞いてなかったんですが、理由の1つはおそらく他の人が手を加えるときにどこを見ればいいかわからないからだと思います。

例えばこの部分で↓

def index
	@current_date = Date.today.strftime("%Y年%-m月%-d日")
end

日付の表示の仕方を変えたいなってなったとき、普通なら該当するビューのhtmlを覗きにいきますよね。でも、ビューには日付の表示に関わる記述がないわけです。

もちろん、これは簡単なコードなので容易にコントローラーが怪しいってことはわかるんですけど、アプリケーションが複雑になればなるほど、どこを参照していいかがわからなくなる...。

動くプログラムではあるけれども、良いプログラムでない。

経験が浅いうちはとくに、書くコードがどの役割を持っていて、どこに書くのが適切なのか意識しておきたいですね!