第1回クソコードレビュー大会解説！

#はじめに
この記事は僕参加しているグループ内でプログラミングの知識の底上げのために行った大会の解説のための記事です。
良ければみなさんもやって見てください！
```https://github.com/takehanKosuke/bad_cord_test_of_rails```

そしてやった後はぜひ感想やなどのフォードバックを何かしらで教えていただけると嬉しいです！


#指摘して欲しかったジャンル
１. とりあえず直さないといけない
２. きれいなコーディング(レベル１)※フォーマット周り
３. きれいなコーディング(レベル２)※gemのメソッドとか
＝＝＝＝ここより下は独学でプログラミングしてたら多分かなり難しいと思っている＝＝＝＝
４. きれいなコーディング(レベル３)※処理回数を減らす的なやつ
５. セキュリティー対策※XSS

#問題項目一覧（全20問）
- 余計な文字列が出現している
- 正常に式展開がされていない
- 記事の削除ができない
- userのarticlesのアソシエーションにdependent: :destroyをつけていない
- バリデーションを掛けていない
- save/update/destroyでエラーハンドリングをしていない
- userのenumであるroleカラムにdefault値をつけていない
- 他の人になりすましてデータを作成更新削除ができてしまう
- seeds.rbのcreateに「!」をつけてない
- インデントが揃っていない
- シングルクォーテーションとダブルクォーテーションが混在
- new/editでの共通部分を部分テンプレート化していない
- article_pathの引数が「article.id」になっている
- enumの検索メソッドを使用していない
- authenticate_user!を使用していない
- n+1問題が発生している
- ビジネスロジックをcontrollerに書いている
- デメテルの法則を使用していない
- Articleにindexを貼っていない
- XSS（クロスサイトスクリプティング）攻撃からの脆弱性がある

##１、とりあえず直さないといけない
 - 余計な文字列が出現している
 - 正常に式展開がされていない
 - 記事の削除ができない
 - userのarticlesのアソシエーションにdependent: :destroyをつけていない
 - バリデーションを掛けていない
 - save/update/destroyでエラーハンドリングをしていない
 - statusにデフォルト値をつけていない
 - 他の人になりすましてデータを作成更新削除ができてしまう
 - seeds.rbのcreateに「!」をつけてない


###余計な文字列が出現している
```erb:views/articles/index.html.erb
<!--修正前-->
</tr>
<%= @articles.each do |article| %>
　　<tr>

<!--修正後-->
</tr>
<% @articles.each do |article| %>
　　<tr>
```
<img width="1429" alt="スクリーンショット 2019-03-31 13.43.52.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/1beb0dfc-47ed-a175-964b-9a88155bbe17.png">
これはもう解説不要な気がしますが、ここに”＝”はいらないです。
気をつけましょうw


###正常に式展開がされていない
```erb:view/articles/index.html.erb
<!--修正前-->
<td><%= '#{article.pv}pv' %></td>

<!--修正後-->
<td><%= "#{article.pv}pv" %></td>
```

![スクリーンショット 2019-04-02 1.28.27.png](https://qiita-image-store.s3.amazonaws.com/0/281558/3ac808a9-d288-bff1-7f7a-45892cc42824.png)

rubyでは式展開を行うときにには *シングルクォーテーション* ではなく *ダブルクォーテーション* を使う必要があります。

じゃあそんなこと言うならシングルクォーテーション使わなければいいんじゃね？って話になりそうですが、、、

下の記事にもあるようにシングルクォーテーションの方がわずかながら処理スピードが速くなるそうなので、処理速度をめちゃめちゃ意識した場合はその辺のことも気をつけていければいいかなと思います。
https://qiita.com/Kenji_TAJIMA/items/78555053a36c214be350

###記事の削除ができない

```erb:view/articles/index.html.erb
<!--修正前-->
<td><%= link_to '削除', article_path(article.id), data: { confirm: "#{article.title}を削除しますか？" } %></td>

<!--修正後-->
<td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>
```

railsの**path** は何も指定しない場合getメソッドを呼んでしまうためdeleteメソッドを使う場合はしっかりと**method: :delete**というようにメソッド名を指定しないといけません
今回の場合method名を指定しない場合はarticleのshowアクションに移動します

###userのarticlesのアソシエーションにdependent: :destroyをつけていない

```ruby:models/user.rb
#修正前
class User < ApplicationRecord
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :validatable

  has_many :articles

  validates :name, presence: true

  enum role: { normal: 1, admin: 2 }
end

#修正後
class User < ApplicationRecord
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :validatable

  has_many :articles, dependent: :destroy  # 追記

  validates :name, presence: true

  enum role: { normal: 1, admin: 2 }
end
```

もしこのdependent: :destroyをつけなかった場合アドミンユーザーでユーザーの削除を行うと
<img width="1435" alt="スクリーンショット 2019-03-31 13.19.40.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/8299f6ad-77b7-1652-42c1-d069e21c08bb.png">
こんな感じのエラーが返されると思います。
これはどういうことかというと、、、
記事の一覧を表示させるときにユーザーの名前を出しているのですが、その記事に紐づくユーザーが削除されてしまっているため参照するユーザー名がないよ〜
というようなエラーです。

これを解決するためには、ユーザーを削除したときにそれと一緒にユーザーの作った記事も一緒に削除するというのが一般的ですので、
今回はuserのarticlesのアソシエーションにdependent: :destroyをつけてuserが削除された時にそのユーザーに紐付く記事も一緒に削除しています

###バリデーションを掛けていない
```ruby:models/article.rb
#修正前
class Article < ApplicationRecord
  belongs_to :user
end

#修正後
class Article < ApplicationRecord
  belongs_to :user

  validates :title, presence: true
  validates :body, presence: true
end
```

記事を作成する際タイトルや内容が空だと不自然ですよね？
バリデーションを掛けないとタイトルや内容が空のまま記事が作成できてしまいます。
今回はデータがあることを強要する*validates*を使ってバリデーションをかけています。

バリデーションについての詳細は↓の記事がかなりよかったので気になるひとは是非読んでみてください
https://qiita.com/shunhikita/items/772b81a1cc066e67930e


###save/updateでエラーハンドリングしていない

```ruby:controllers/articles_controller.rb
#修正前
  def create
    @article = Article.new(article_params)
    @article.save
    redirect_to root_path
  end

  def update
    @article = Article.find(params[:id])
    @article.update(article_params)
    redirect_to root_path
  end

#修正後
def create
   @article = Article.new(article_params)
   if @article.save
     redirect_to @article, flash: { success: 'articleが作成されました' }
   else
     render :new
   end
end

def update
   if @article.update(article_params)
     redirect_to @article, flash: { success: 'articleが更新されました' }
   else
     render :edit
   end
end
```

そもそもエラーハンドリングって？
>エラーハンドリングとは、プログラムの処理中に処理が妨げられる事象が発生した際、その処理をエラーとして対処する処理のことである。 例外処理とも呼ばれる。
>(出典：weblio)

今回の場合でエラーハンドリングをちゃんとしないと、、、
<img width="497" alt="スクリーンショット 2019-03-17 20.57.06.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/c1795ba8-6f02-d7f8-4d52-515d974ff002.png">
<img width="502" alt="スクリーンショット 2019-03-17 20.57.34.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/9ef8d076-e230-7245-bae1-427abe030714.png">
このように新規作成ボタンを押してもボタンが無効になるだけでデータが作成されないです


なので、save,updateなどの処理を挟む時は必ずif文を使ってエラーハンドリングをして
データが作成されない時のフォローをしてあげましょう。

### userのenumであるroleカラムにdefault値をつけていない
```ruby:db/migrate/20190220135142_devise_create_users.rb
#修正前
 # カスタムカラム
t.string :name, null: false
t.integer :role, null: false

#修正後
 # カスタムカラム
t.string :name, null: false
t.integer :role, null: false, default: 1
```

もしroleカラムにデフォルト値をつけなかった場合ユーザーを新規に作成するときに以下のようなエラーが返されると思います。
<img width="1440" alt="スクリーンショット 2019-03-31 13.16.41.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/b9d648b3-2f0b-2062-4a28-d5ec22b12340.png">

これはどういうことかというと、、、「roleカラムが指定されてないんだけど〜」っていうエラーですね
今回ユーザーのroleカラムはユーザーの新規登録時に操作させません
（というか、新規登録時にユーザー自身アドミンユーザーかノーマルユーザーかを決められたらアドミンユーザーの意味がw）
ですが、ユーザーのroleカラムは全てのユーザーが持っていなければならないのです。
そのようなときは最初からroleにデフォルト値を設定してフォローするのがベターだと思われます。

またenumは基本的にデフォルト値をつけるのが慣習となっているので（多分w）
そう言った意味でもデフォルト値をつけるべきでしょう

###他の人になりすましてデータを作成更新削除ができてしまう
```controller/articles_controller.rb
#修正前
#一部省略

  def new
    @article = Articles.new
  end

  def create
    @article = Article.new(article_params)
    @article.save
    redirect_to root_path
  end

  def edit
    @article = Article.find(params[:id])
  end

  def update
    @article = Article.find(params[:id])
    @article.update(article_params)
    redirect_to root_path
  end

  def destroy
    @article = Article.find(params[:id])
    @article.destroy
    redirect_to root_path
  end

#一部省略

def article_params
  params.require(:article).permit(
    :title,
    :body,
    :user_id
  )
end


#修正後
before_action :article_author, only: %i[edit update destroy]

#一部省略

def new
  @article = current_user.articles.new
end

def create
  @article = current_user.articles.new(article_params)
  if @article.save
    redirect_to @article, flash: { success: 'articleが作成されました' }
  else
    render :new
  end
end

#一部省略
private
def article_author
  @article = current_user.articles.find(params[:id])
end

def article_params
  params.require(:article).permit(
    :title,
    :body,
  )
end
```

どういうことか実際にやってみましょう！
![スクリーンショット 2019-03-31 13.32.52.png](https://qiita-image-store.s3.amazonaws.com/0/281558/b3763f46-1ad9-45d7-208f-cccc8da66729.png)
新規作成の画面を開き、開発者ツールを開き上記の写真のuserIDを指定しているところを見つけましょう。
そこのIDを今ログインしているユーザー以外のIDを指定して新規作成をおします。
すると以下のようにログインしているユーザー以外で記事を作成できてしまいます
![スクリーンショット 2019-03-31 13.33.12.png](https://qiita-image-store.s3.amazonaws.com/0/281558/e607b259-3662-466a-915e-b19b5d868db1.png)
この現象を避けるため、今回はuserとarticleとの紐付けをhidden_fieldで現在のユーザーのidを送るのではなく、

```
@article = current_user.articles.new
```

というような記述の仕方でユーザーと記事の紐付けを行いました。

### seeds.rbのcreateに「!」をつけてない
```seeds.rb
#修正前
puts "ユーザー作成中"
User.create(
  [
    {
      name: 'kosuke',
      email: 'test1@test.com',
      password: '111111',
      role: 2,

#修正後
puts "ユーザー作成中"
User.create!( #「!」をつけた
  [
    {
      name: 'kosuke',
      email: 'test1@test.com',
      password: '111111',
      role: 2,
```

seedファイルのcreateには「!」をつけないとエラーを吐いてくれません
つまり、seedが失敗してもそれに気づくことが難しくなってしまうのでseedファイルには必ず「!」をつけましょう

本当にエラー吐かないの？！って思う人は```rake db:seed```を「！」をつけたバージョンとつけなかったバージョンの２回叩いてみるとわかると思います

##２、きれいなコーディング（レベル１）
- インデントが揃っていない
- シングルクォーテーションとダブルクォーテーションが混在
- new/editでの共通部分を部分テンプレート化していない
- article_pathの引数が「article.id」になっている

###インデントが揃っていない

```erb:views/articles/index.html.erb
<!--修正前-->
<table>
  <tr>
    <th>タイトル</th>
      <th>内容</th>
      <th>pv数</th>
    <th>ライター</th>
    <th></th>
    <th></th>
  </tr>
  <% @articles.each do |article| %>
    <tr>
        <td><%= link_to '#{article.title}', article_path(article.id) %></td>
        <td><%= article.body.html_safe %></td>
        <td><%= "#{article.pv}pv" %></td>
        <td><%= "#{article.user.name}" %></td>
        <td><%= link_to "編集", edit_article_path(article.id) %></td>
      <% if article.user == current_user %>
        <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>
      <% end %>
      </tr>
    <% end %>
</table>

<!--修正後-->
<table>
  <tr>
    <th>タイトル</th>
    <th>内容</th>
    <th>pv数</th>
    <th>ライター</th>
    <th></th>
    <th></th>
  </tr>
  <% @articles.each do |article| %>
    <tr>
      <td><%= link_to '#{article.title}', article_path(article.id) %></td>
      <td><%= article.body.html_safe %></td>
      <td><%= "#{article.pv}pv" %></td>
      <td><%= "#{article.user.name}" %></td>
      <td><%= link_to "編集", edit_article_path(article.id) %></td>
      <% if article.user == current_user %>
        <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>
      <% end %>
    </tr>
  <% end %>
</table>
```

初めてインデントとかを気にし始めた時はかなり大変さを感じるかもしれないですが、それに慣れてしまうと、むしろインデントが揃ってないと違和感しか感じないですw

ちなみに各エディタにはインデントとかを揃えてくれるプラグインが存在するはずなので是非調べて見てください。
僕自身は「atom」を使っているのですが、「atom」では「atom-beautify」と言うプラグインがあり、それを使うと
```control + option + B```
で自動で整形してくれます。

###シングルクォーテーションとダブルクォーテーションが混在
```erb:views/articles/index.html.erb
<!--21行目-->
<!--修正前-->
<td><%= link_to "編集", edit_article_path(article.id) %></td>
<% if article.user == current_user %>
  <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>

<!--修正後-->
<td><%= link_to '編集', edit_article_path(article.id) %></td>
<% if article.user == current_user %>
  <td><%= link_to '削除', article_path(article.id), method: :delete, data: { confirm: "#{article.title}を削除しますか？" } %></td>
```

プログラミングをする際はできるだけ書き方を統一しましょう。
今回はシングルクォーテーションに揃えていますが、プログラム全体でダブルクォーテーションに揃えているのであれば、ダブルクォーテーションで揃えるのが適切だと思います。

###new/editの共通部分が部分テンプレート化していない

```erb:views/articles/new.html.erb
<!--修正前-->
<h1>新規作成</h1>
<% if @article.errors.any? %>
  <ul>
    <% @article.errors.full_messages.each do |message| %>
      <li><%= message %></li>
    <% end %>
  </ul>
<% end %>

<%= form_with model: @article, local: true do |f| %>
  <%= f.hidden_field :user_id, value: current_user.id %>
  <div>
    <%= f.label :title %>
    <%= f.text_field :title %>
  </div>
  <div>
    <%= f.label :body %>
    <%= f.text_area :body %>
  </div>
  <%= f.submit '作成' %>
<% end %>

<!--修正後-->
<%= render 'form', article: @article %>
```

``````erb:views/articles/edit.html.erb
<!--修正前-->
<h1>編集</h1>
<% if @article.errors.any? %>
  <ul>
    <% @article.errors.full_messages.each do |message| %>
      <li><%= message %></li>
    <% end %>
  </ul>
<% end %>

<%= form_with model: @article, local: true do |f| %>
  <%= f.hidden_field :user_id, value: current_user.id %>
  <div>
    <%= f.label :title %>
    <%= f.text_field :title %>
  </div>
  <div>
    <%= f.label :body %>
    <%= f.text_area :body %>
  </div>
  <%= f.submit "更新" %>
<% end %>

<!--修正後-->
<%= render 'form', article: @article %>
```

※新規作成

```erb:views/articles/_form.html.erb
<h1><%= current_page?(new_article_path)? "新規作成" : "編集" %></h1>
<%= render 'layouts/errors', object: @article %>

<%= form_with model: article, local: true do |f| %>
  <%= f.hidden_field :user_id, value: current_user.id %>
  <div>
    <%= f.label :title %>
    <%= f.text_field :title %>
  </div>
  <div>
    <%= f.label :body %>
    <%= f.text_area :body %>
  </div>
  <%= f.submit "#{ current_page?(new_article_path)? "新規作成" : "更新" }" %>
<% end %>


```

※新規作成

```erb:views/layouts/_errors.html.erb
<% if object.errors.any? %>
  <ul>
    <% object.errors.full_messages.each do |message| %>
      <li><%= message %></li>
    <% end %>
  </ul>
<% end %>
```

新規登録と編集画面のフォームは全く一緒なので部分テンプレートとして切り出し、
エラーメッセージは他のところでも使う可能性があるので、さらに別のファイルに切り出して実装を行いました

今回は、「新規作成/編集」のところもcurrent_page?メソッド（現在のページのパスを返すrailsのメソッド）を使って変化させていますが、多少やりすぎ感は否めないかなとも思ってますw


###article_pathの引数が「article.id」になっている
```erb:views/articles/index.html.erb
<!--修正前-->
<td><%= link_to article.title, article_path(article.id) %></td>


<!--修正後-->
<td><%= link_to article.title, article_path(article)%></td>
```

idを引数として渡す場合はオブジェクトそのものを渡してあげることもできる。
かなり細かいところではあるが、意外と重宝することがある




##３、きれいなコーディング（レベル２）

- enumの検索メソッドを使用していない
- authenticate_user!を使用していない

###enumの検索メソッドを使用していない
```erb:views/users/show.html.erb
#修正前
<% if current_user.role == "admin" %>

#修正後
<% if current_user.admin? %>
```

enumを使うことで```current_user.admin?```というように記述することができよりシンプルな記述をすることができます。

参考文献
https://qiita.com/shizuma/items/d133b18f8093df1e9b70


###authenticate_user!を使用していない
```ruby:application_controller.rb
#修正前
def user_sign_in?
  if user_loged_in?
    redirect_to :user_session_path
  end
end

#修正後
before_action :authenticate_user!
```

gemのdeviseを使うことで様々なメソッドを使うことができるようになります


| メソッド | 用途 |
|:--|:--|
| before_action :authenticate_user! | コントローラーに設定して、ログイン済ユーザーのみにアクセスを許可する |
| user_signed_in? | ユーザーがサインイン済かどうかを判定する |
| current_user | サインインしているユーザーを取得する |
| user_session | ユーザーのセッション情報にアクセスする |

詳細は↓の記事から、、、
https://qiita.com/tobita0000/items/866de191635e6d74e392#user_session


##４、きれいなコーディング（レベル３）

- n+1問題が発生している
- ビジネスロジックをcontrollerに書いている
- デメテルの法則を使用していない
- Articleにindexを貼っていない

###n+1問題が発生している
```ruby:controllers/articles_controller.rb
#修正前
  def index
    @articles = Article.all
  end

#修正後
  def index
    @articles = Article.all.includes(:user)
  end
```

そもそも**n+1問題**って？  
めちゃめちゃ簡単に言うと無駄なSQL（データベースからデータを取ってくる命令）が発行されてしまっている現象です。
どれくらい違うかと言うと、、、

**修正前**
<img width="1082" alt="スクリーンショット 2019-03-30 13.10.03.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/34079284-b1c0-6b54-2077-53b13adad017.png">


**修正後**
<img width="980" alt="スクリーンショット 2019-03-30 13.11.14.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/3cc919f3-a97e-24ef-f873-e4a0a525e80e.png">


たった10個の初期データを表示するだけにも関わらず
ここまで発行量に差があります。

じゃあ、なんでこなんでこんなに違いがあるのかと言うと、、、
調べましょう！！！~~説明がめんどくさかったとかそう言うのじゃないんだからね！~~

まあでもとりあえず参考文献だけは置いておきます
https://qiita.com/TsubasaTakagi/items/8c3f4317ad917924b860


###デメテルの法則を使用していない
```ruby:models/article.rb
#追加
delegate :email,
         :name,
         to: :user,
         prefix: true
```

```erb:views/articles/_article_list.html.erb
<!--修正前-->
<tr>
  <td><%= link_to article.title, article_path(article) %></td>
  <td><%= article.body.html_safe %></td>
  <td><%= "#{article.pv}pv" %></td>
  <td><%= article.user.name %></td>

<!--修正後-->
<tr>
  <td><%= link_to article.title, article_path(article) %></td>
  <td><%= article.body.html_safe %></td>
  <td><%= "#{article.pv}pv" %></td>
  <td><%= article.user_name %></td>
```

**デメテルの法則**とは、
>あるオブジェクトAは別のオブジェクトBのサービスを要求してもよい（メソッドを呼び出してもよい）が、オブジェクトAがオブジェクトBを「経由して」さらに別のオブジェクトCのサービスを要求してはならない。(wikipediaより引用)

まあよくわかんないと思うのでめちゃめちゃ簡単にいうと、、、
**「.」を2つ以上繋げんなよ！**って感じですな（ただこの解釈で固まりすぎると行きすぎると死ぬことあるのでその辺は注意、、、）

じゃあ、今回みたいに ```article.user.name``` みたいに書かなきゃいけない時はどうすればいいの？ってことが起きると思います。

```ruby:models/article.rb
  def user_name
    user.name
  end
```

その時は上のようにarticleのモデルにこんな感じのメソッドを書いてあげれば
```article.user_name``` みたいな感じで「.」を一つで呼ぶことができます。（※正確にいうとArticleモデルのメソッドとしてuser名を引っ張ってこれる）
なので今回の実装では**railsのActiveSupportのコンポーネントのdelegate**を使って実装を行いました。

↓参考文献
https://railsguides.jp/active_support_core_extensions.html#%E3%83%A1%E3%82%BD%E3%83%83%E3%83%89%E3%81%AE%E5%A7%94%E8%AD%B2

何を言ってるのかよくわからん！っていう人は、
**「.」を2つ以上繋げるのはあんまり良くないんだな〜**みたいな認識でオッケーかと思います。



###Articleにindexを貼っていない
```ruby:db/migrate/20190220133546_init.rb
#修正前
create_table :articles do |t|
  t.string :title, null: false
  t.text :body, null: false
  t.integer :pv, null: false, default: 0

  t.integer :user_id, null: false
  t.timestamps
end

#修正後
create_table :articles do |t|
  t.string :title, null: false
  t.text :body, null: false
  t.integer :pv, null: false, default: 0

  t.integer :user_id, null: false
  t.timestamps
end
add_index :articles, [:user_id] #追記
```
**index**を張るってどういうこと？
プログラミングでindexというといろんな意味がありますが、、、
今回のindexの意味は**検索を早くする仕組みのことです**

なんでindexを張ると検索が早くなるのかは下の記事を見ていただくとして、、、
indexは基本的には外部キーに貼っておくといいとされているらしいので、よくわからん！！！って人もとりあえず外部キーには貼っておきましょう

参考文献
〜簡単に分かりたい人向け〜
https://wa3.i-3-i.info/word11906.html
〜割とちゃんと分かりたい人向け〜
https://www.atmarkit.co.jp/ait/articles/1703/01/news199.html


###ビジネスロジックをモデルにかいていない
```controller/articles_controller.rb
#修正前
def show
  @article = Article.find(params[:id])
  @article.pv += 1
  @article.save
end

#修正後
def show
  @article = Article.find(params[:id])
  Article.increment_pv(@article)
end
```

```models/article.rb
def self.increment_pv(article)	
  article.pv += 1	
  article.save	
end
```

そもそも**ビジネスロジック**ってなんやねん！
>ビジネスロジック（英: business logic）は、データベースとユーザーインタフェース間の情報のやりとりを制御する手順といったようなものを指す（技術的でない）用語である。(wikipediaより引用)

ってことだそうです、、、いや、どういうことやねんwww

まあちょっとよくわからないので
逆にcontrollerの役割について確認したいと思います。
<img width="905" alt="スクリーンショット 2019-03-31 12.24.19.png" src="https://qiita-image-store.s3.amazonaws.com/0/281558/edaad16f-5a71-0b59-ddcd-1769c0ee44ff.png">
上の図はMVCフレームワークの大まかな流れなのですが
ズバリコントローラーの役割とは、、、

***送られてきたリクエストを適切なアクションに振り分けること！***

です。
そして、データベースとのやりとりは**model**が行うことになっています。

そこで改めて該当のコードを見てみると、、、

```ruby
def show
  @article = Article.find(params[:id])
  @article.pv += 1
  @article.save
end
```
というようになっています。

ここで行なっている処理は
1. 該当のarticleを持ってくる
2. 持ってきたarticleのPV数を1を足す

まず、１の「該当のarticleを持ってくる」というところですが、
先ほどの話からするとこの作業は本来modelが担当するべきところですが、```@article = Article.find(params[:id])```
となっているので、まるでコントローラがDBにアクセスているように見えますが、このrailsの**find**というメソッドは実はmodels/user.rbで継承されているActiveRecordのメソッドを呼び出しているのです！
なのでちゃんとMVC通りmodelでデータベースとやりとりを行なっていたんですね！（この辺は話すと長くなるのでこれくらいで割愛w）

そして、2の「持ってきたarticleのPV数を1を足す」という処理ですが、この作業も本来はmodelが担当するべきところですが修正前のコードではcontrollerでpv数を１足しています(ちなみに**save**もActiveRecordのメソッドです)
コントローラは処理を振り分けることなので、pv数に１を足すという処理をここで行なってしまうとMVCの考え方に反してしまいます。
なので今回はこれらの処理はメソッドとして切り出してmodelファイルに記述して、それを呼びだすという実装にしています。



ってまあ、ぶっちゃけよくわからん！ってなる人も多いと思います汗
なのでそんな人は、とりあえず、、、

よくわかんないけど、
***普段使ってる「find」とか「save」とかはmodelファイルで定義しているメソッドをcontrollerで呼び出して使ってるだけなんだなぁ〜***

ってことだけ頭の片隅にでも置いておいてもらえるといいかなと思います



ビジネスロジックをさっくり知りたい
https://wa3.i-3-i.info/word13666.html

もっと知りたい
〜MVCモデル〜
https://qiita.com/tshinsay/items/5b1724baf32b8b5113c2

〜ActiveRecord〜
https://railsguides.jp/active_record_basics.html

##５、セキュリティー系

###XSS（クロスサイトスクリプティング）攻撃からの脆弱性がある
```erb:views/articles/_article_list.html.erb
<!--修正前-->
<% articles.each do |article| %>
  <tr>
    <td><%= link_to article.title, article_path(article) %></td>
    <td><%= article.body.html_safe %></td><!--⬅︎修正するところ-->
    <td><%= "#{article.pv}pv" %></td>
    <td><%= article.user_name %></td>

<!--修正後-->
<% articles.each do |article| %>
  <tr>
    <td><%= link_to article.title, article_path(article) %></td>
    <td><%= simple_format(article.body) %></td> <!--⬅︎修正した-->
    <td><%= "#{article.pv}pv" %></td>
    <td><%= article.user_name %></td>
```
**クロスサイトスクリプティング**ってなんぞ？
こればっかりは実際にやって見たほうが早いのでやって見ましょう！

```
<script>location.href="https://qiita.com/"</script>
```
これをbodyに書いて記事を作成してみましょう
すると、、、
おそらく記事の一覧画面に言った時にqiitaのトップページに勝手にリダイレクトされると思います。
今回はこれがqiitaのトップページであるため、問題はありませんが例えばこれの飛ばされる先が悪質なサイトである可能性は十分にあるわけです。。。
ではこれらはどのようにすれば防ぐことができるのでしょうか？

キーワードとしては **サニタイズ** です

サニタイズとは簡単にいうと特別な文字を他の文字に置き換えちゃおうぜって感じのことです。
railsでは基本的にrubyタグ（<%= %>こんな感じのやつ）を使うことでサニタイズをしてくれるのですが ```.html_safe``` を使うとサニタイズしてくれなくなってしまいます。
（safeって書いてあるのに全然安全じゃないってのがツッコミどころ満載な感じですが、あまりにツッコミどころ多いので逆に覚えやすいw）

でも、だからと言って```<td><%= article.body %></td>``` と書いてしまうと、改行がうまくされませんそのため今回は**simple_format**というrailsのヘルパーを用いて実装を行っています。

参考文献
〜サニタイズについて〜
https://wa3.i-3-i.info/word16265.html
〜simple_formatについて〜
http://railsdoc.com/references/simple_format


##お疲れ様です！！！
みなさん、問題の回答から解説の読了までお疲れ様でした！！！
今回の問題楽しんでいただけましたでしょうか？また何か新しい発見や学びはありましたでしょうか？

もし少しでも発見や学びがあればとても嬉しいなと思います！

というか今回の問題個人的にはなかなか難しいと思っていて、自分で作っていながらこれ俺が出されても１発で満点取れます！みたいなことは口が裂けても言えないなぁ〜って思っていますので、全然点数が取れなかった人もあまり気にせずにまたゆっくり一緒に勉強していければなと思います！

最後に、、、意見や質問、感想などあったら教えていただけると幸いです！

　　　

##参考文献まとめ
- シングルクォーテーション、ダブルクォーテーション
https://qiita.com/Kenji_TAJIMA/items/78555053a36c214be350

- バリデーション
https://qiita.com/shunhikita/items/772b81a1cc066e67930e

- enum
https://qiita.com/shizuma/items/d133b18f8093df1e9b70

- devise
https://qiita.com/tobita0000/items/866de191635e6d74e392#user_session

- n+1問題
https://qiita.com/TsubasaTakagi/items/8c3f4317ad917924b860

- デメテルの法則
https://railsguides.jp/active_support_core_extensions.html#%E3%83%A1%E3%82%BD%E3%83%83%E3%83%89%E3%81%AE%E5%A7%94%E8%AD%B2

- indexを張っていない
  - 簡単に分かりたい人向け  
  https://wa3.i-3-i.info/word11906.html
  - 割とちゃんと分かりたい人向け  
  https://www.atmarkit.co.jp/ait/articles/1703/01/news199.html

- モデルにビジネスロジックを書いている
 - ビジネスロジックをさっくり知りたい  
  https://wa3.i-3-i.info/word13666.html
 - MVCモデル  
  https://qiita.com/tshinsay/items/5b1724baf32b8b5113c2
 - ActiveRecord
  https://railsguides.jp/active_record_basics.html

- XSSの脆弱性がある
 - サニタイズについて
  https://wa3.i-3-i.info/word16265.html
 - simple_formatについて
  http://railsdoc.com/references/simple_format