Conversation
|
厳しいことを言ってしまうと、大体間違ってるのでコメントどうしようか悩むレベル。 |
|
う、、、すいません。まずそもそもの方針をしっかりと理解したいです。 |
|
最も大きな問題はテーブルの作り方が間違ってる、つまり、データ構造の捉え方が間違っている。 # FeedはChannelに名前変更したほうが分かりやすそう
class Feed < ApplicationRecord
# title, descriptionを追加
# attribute :url, :title, :description
has_many :items
end
class Item
# attribute :title, :link, :description
belongs_to :feed
end |
app/controllers/feeds_controller.rb
Outdated
| private | ||
| def feed_params | ||
| params.require(:feed).permit(:url) | ||
| params.require(:feed).permit(:url, channel_attributes: [:channel_title, :description, :item_title, :link, :pubdate, :feed_id, :id]) |
There was a problem hiding this comment.
strong parameterがなんで生まれたのかの小話。
Railsだとフォームとかで送られてきたパラメータを例えば User.new(params[:user]) みたいに雑に受けるのが昔は一般的だったのですが、これの問題として、ユーザー操作によって更新されると困るものまで操作されえてしまう、という問題がありました。
フォームでしか情報更新したことないとよくわからないかもしれませんが、ブラウザ上でHTMLをいじればユーザーが好きなnameのフォームも作れますし、また、curlなどを使って直接URL叩いてフォーム送信することもできるので、例えばusersにadminというカラムがあってそれで管理者権限を管理している場合など、 admin: true なデータを勝手に送られると、意図せずユーザーに管理者権限を取られてしまうことがありえました。
Rails3までは attr_accessible という機能を使ってこれを防いでいましたがちょっと機能的によくなかった面もあり、今はStrong Parameterというのを使っています。(この、 params.permit.require ってやつ )
で、このコードだと id なんかがstrong parameterでpermitしていますけれども、これって要するに「ユーザーがidを勝手に変更していい」という意思表示になるわけで、そんなことを許すことは絶対にないので書いちゃダメです
|
データの捉え方が間違ってるの、回り道っぽいですけれども着実な手段として、まずは全てのデータを一つのテーブルで表現してみて、そこから第3正規化くらいまでしてみるのがいいと思います。 |
|
正規化をしていく過程は「頭で考えるだけ」ってのはやめたほうがいいです。紙でもスプレッドシートでもなんでもいいので、書き出していくのが大事 |
|
いつも丁寧にすいません:sweat_drops: 今、idを書く危険性を知りもしなかったことに危険性を感じています:sweat: |
フォームなどで入力して、paramsから取り出して、DBに入れる、という流れしか経験しておらず、そこからの応用ができるほどにはまだ個々の処理について理解できていないため、パターンから外れたことで混乱しているように見受けられます。 ヒントとして、 最新の情報を取得するボタンを簡単に実装するならこんな感じのものになると思います。 <%= button_to '最新の情報を取得', fetch_items_feeds_path %>FeedsController < ApplicationController
# (省略)
# 最新の情報を取得ボタンを押して呼ばれるアクション
def fetch_items
# feed#url のRSSから記事情報を登録して Item (Feed#items) に追加する処理を書く。
# データを取ってくるところまでは Feed.channels でできているので、取ってきたデータをDBに入れることを考えればよい
redirect_to feeds_url, notice: '最新の情報を取得しました。'
end
end |
|
@itkrt2y 「一事実一箇所」という考え方を念頭に置いて自分の設計を見ると、無駄だらけの設計でした、、、:dizzy_face: |
|
@takeyuweb ヒント、ありがとうございます:bow: |
|
まず、テーブルの設計を修正していきます。(テーブル名の変更も含め feed → channel) |
|
一旦テーブル修正などを行い、pushしました。テスト(CircleCI)のエラーは調査中です。(外部キー関連のようで、、、)test_helper.rbの fixtures :all をコメントアウトすると通りますが、果たしてそれが良いことなのかも調査中です。。。:cold_sweat: |
| @@ -0,0 +1,6 @@ | |||
| class RenameFeedsToChannels < ActiveRecord::Migration[5.2] | |||
| def change | |||
| rename_table :channels, :items | |||
There was a problem hiding this comment.
そもそも create_items でmigrationを作れば、このrenameも不要
There was a problem hiding this comment.
既存のchannelsテーブルの内容を修正して、新しくitemsテーブルを作り、機を見て(チーム開発なら)feedsテーブルを削除という順序だったということでしょうか?:sweat_drops:
There was a problem hiding this comment.
いや、以下の流れですね。削除いらない
- feeds => channelsにrename
- channelsテーブルにカラム追加
- itemsテーブル作成
There was a problem hiding this comment.
feedsテーブル, channelsテーブル(迷走して作っていたテーブル)の二つが存在していた状況だったので、feedsテーブルをrenameすると同じ名前のものが2つになるため、作れないのかと思ってしまっていました:sweat_drops:
There was a problem hiding this comment.
迷走中に作ったテーブルはdb:rollbackで消せばええんやで
まだマージされていなくて @shiraryu さんのところにしかないのでできることですが
There was a problem hiding this comment.
そういうことだったんですね:sweat_drops:
マージされているかいないかの部分をもっと意識して作業します。
title, descriptionカラム追加の際に、新しくマイグレーションファイルを作って行わなかったことも含め、気をつけます:bow:
修正(整えて)いきます。
app/models/channel.rb
Outdated
| class Channel < ApplicationRecord | ||
| belongs_to :feed | ||
| has_many :items, dependent: :destroy | ||
| accepts_nested_attributes_for :items |
|
|
||
| private | ||
| def channel_params | ||
| params.require(:channel).permit(:url, :title, :description, items_attributes: [:title, :link, :pubdate]) |
There was a problem hiding this comment.
:url 以外について、なぜ追加で指定したのか、理由を説明できますか?
データ構造の修正まで、ということなら今はいいか
There was a problem hiding this comment.
ふむ、では改めて
上で出た accepts_nested_attributes_for :items も関連しますが、なぜこれらを追加したか理由を説明できますか?
There was a problem hiding this comment.
いえ、説明できません:sweat_drops:
これらは「最新の情報を取得する」ボタンを作成するということを試みていた際に迷走した結果でして、、、:cold_sweat:
There was a problem hiding this comment.
なるほど、ではいったん消して、先にヒントとして僕が出したものを実装していくうえで必要になったらその時に書いてください。
今はまだ難しいかもしれませんが、プログラミングにおいて『理由を説明できないコード』は基本的にあってはならないと思っていてください。
コードを読む際はコードの理由を考えるので、こういった理由を説明できないコードがあると理解が難しくなります。
There was a problem hiding this comment.
ってコメントしましたが、この箇所については単に削除漏れと理解したので、消せばOKで、日頃そういうことを意識してコードを書くよう心がけてください、というお話です。
There was a problem hiding this comment.
承知しました!ただ、「ドキッ」としました。
「説明できないや」と。。。
つまり、「最新の情報を取得する」ボタンを作成する試みをしていた際に「当てずっぽう」なことをしていたということが問題ですね。『理由を説明できないコード』はあってはならないということを肝に命じておきます。ありがとうございます。
| create_table :feeds do |t| | ||
| t.string :url, null: false, default: '', comment: 'RSSフィードのURL' | ||
| t.string :title | ||
| t.string :description |
There was a problem hiding this comment.
一人プロジェクトなのでわかってやってるなら別にいい話になりますが、migrationって一度流れたものは繰り返し流れないので、仮に複数人でこの開発を行っていた場合、このmigrationが実行済みの人はこの変更を反映できなくなります。
新規にmigrationファイルを作成するのが正しい手順になります。
There was a problem hiding this comment.
開発云々よりも、本番にあげたのをmigration resetしなければならないのが最も問題か。データを全部消して再実行するしかない。
今は消してもいいやつです。追々学んでいただければ |
|
データ構造はこれでOK |
|
@itkrt2y 「fixtures :all」「データ構造」の件、ありがとうございます! |
|
fixturesの件、ざっくり説明すると以下のようなお話
今はテスト書いてないですし、書くとしてもfixtures使うか微妙なので消しといてOK。真面目に対処するならば、fixtures定義をちゃんとやれば通るようになります。 |
|
@itkrt2y fixturesの説明、わかりやすくありがとうございます:bow: |
test/test_helper.rb
Outdated
| @@ -4,7 +4,6 @@ | |||
|
|
|||
| class ActiveSupport::TestCase | |||
| # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order. | |||
| render 'new' | ||
| end | ||
| channel_id = @channel.id | ||
| Channel.items_save(channel_id) |
There was a problem hiding this comment.
少し理解しづらいかもしれませんが、クラスメソッドとインスタンスメソッドについて調べてみてください。
ruby インスタンスメソッド クラスメソッド などとググれば参考になるものも見つかると思います。(この仕事、ググり力もかなり重要、鍛えましょう
There was a problem hiding this comment.
この行でもう一つ。
ここだと @channel.save が失敗した、つまり、channelが存在しない場合においてもchannelのitemを保存しようとしてしまいます
|
まだ、クラスメソッドとインスタンスメソッドを理解しづらい状況です。 |
app/models/channel.rb
Outdated
| def self.items_save(channel_id) | ||
| channel = find(channel_id) | ||
| def items_save(channel_id) | ||
| channel = Channel.find(channel_id) |
There was a problem hiding this comment.
おっ、そうですね、これはChannelクラスインスタンスのitemsを操作するので、Channelクラスのインスタンスメソッドですね。
インスタンスメソッドを使うことで、特定の Channel クラスインスタンスに対して、「items更新しろ」と指示できるわけですね。
一方でクラスメソッドは Channel クラスには関係あるけど、個々のインスタンスのデータは使わないメソッドを定義したいときに使います。ちょっと今ちょうどいい例を思いつきませんが、どんな・誰のデータを扱うか、で使い分ける感じです。
There was a problem hiding this comment.
ただこれ、惜しいです。
ヒントは items_save メソッド中における self はなんでしょうね…というのは、ちょっとまわりくどいかな。
There was a problem hiding this comment.
@takeyuweb すいません。コメントに気づかず、pushしてしまいました:sweat_drops:
再度、コメント内容を考えてみます!
There was a problem hiding this comment.
@takeyuweb ご確認ありがとうございます!
まだモヤっとしているので、整理していきます:bow:
| @channel = Channel.new(channel_params) | ||
| if @channel.save | ||
| @channel.items_save(@channel.id) | ||
| Channel.new.items_save(@channel) |
| @channel = Channel.new(channel_params) | ||
| if @channel.save | ||
| Channel.new.items_save(@channel) | ||
| @channel.items_save(@channel.id) |
| # https://news.yahoo.co.jp/pickup/rss.xml | ||
| # https://rss-weather.yahoo.co.jp/rss/days/7320.xml | ||
| class ChannelsController < ApplicationController | ||
| before_action :set_channel,only:[:new, :fetch_items] |
There was a problem hiding this comment.
fetch_items で @channel = Channel.new じゃだめだから、間違ってるし、
そもそもこれによって何も楽になってないから、わざわざ before_action する必要も特にない
There was a problem hiding this comment.
調べるだけじゃ意味なくて、考えて理解する。
調べてそれっぽいのをコピペしたりするのは、あくまでも理解するまでの過程。
コピペするにしてもなぜ必要なのか?考えて説明できるようにしないとだめです。
コピペプログラマというのも世の中には存在するらしいですが。
There was a problem hiding this comment.
すいません、言葉が悪かったです。現時点でコピペは行っていません。
単純に理解が足りていない状況です。
|
「レシーバを操作する」って概念が全くわかっていない印象 |
|
クラスとインスタンスの違いも全然わかってないので、インスタンスメソッドでクラスメソッドな処理を書いている |
|
|
|
@itkrt2y はい、はっきりと「なるほど」というように違いを認識できずにいます。しかもそれほど意味不明なことをしてる状況でしたか、、、自身で言うのも何ですが、現時点で深刻だと感じるほど理解しにくいのは事実です。 |
|
クラスとインスタンスの違い、基本的な事柄すぎて逆に説明しづらいのですが、RailsのModelに限った話で例えると以下のような感じで、RailsのModelではクラスはテーブルを表し、インスタンスは1レコードを表している。 Channel.create(title: "Channel Title") # createはクラスメソッド。channelsテーブルにレコードを作る
channel = Channel.find(1) # findはクラスメソッド。channelsテーブルからレコードを取得する
channel.title = "Updated Title" # title= はインスタンスメソッド。channelインスタンスのtitileに値を代入する
channel.save # saveはインスタンスメソッド。channelインスタンスの現在の状態をDBに保存するModelじゃなくても基本的にはこういう概念であり、「Dogクラス作って個々の犬を表すインスタンスを作る」みたいなことをする。 |
|
@itkrt2y ありがとうございます!「レシーバを操作する」ということと共に、再度落ち着いて考えます:bow: |
app/models/channel.rb
Outdated
| self.save if self.changed? | ||
|
|
||
| items = channel.items | ||
| items = self.items |
app/models/channel.rb
Outdated
| def items_save | ||
| source_channel = RSS::Parser.parse(self.url).channel | ||
| self.assign_attributes(title: source_channel.title, description: source_channel.description) | ||
| self.save if self.changed? |
There was a problem hiding this comment.
self. 書いてるの、間違ってるというわけでもないのですが、省略できるものは書かないのが一般的で、これら self は全部省略可能です。
self が必要になるのは多分変数代入くらい
title = 'foo' # これはChannel#titleへの代入よりも変数定義が優先される
self.title = 'bar' # selfをつければChannel#itemへの代入になるThere was a problem hiding this comment.
「省略できるものは基本書かない」承知しました!
変数定義と変数代入の違いも押さえておきます!
There was a problem hiding this comment.
☝ のコメント # selfをつければChannel#titleへの代入になる ですね。
もうちょっと厳密にいえば、この場合は代入ではなくて Channel#title=メソッドの呼び出し、でしょうか。
省略できるので不要ですが self. を書いてるの、レシーバを意識できているという点では良い兆候です。
self.assign_attributes は、 self. つまり自分自身を .assign_attributes メソッドで操作するよ、ということですね。
There was a problem hiding this comment.
クラスとインスタンスの違いはもとより、レシーバを意識することやインスタンスメソッド内のselfがオブジェクトとなっているということなど、わかっていないことが多かったと感じています:bow: selfが有るか無いかで大きく意味合いが変わる点、気をつけます:sweat_drops:
app/models/channel.rb
Outdated
| has_many :items, dependent: :destroy | ||
| validates :url, format: /\A#{URI::regexp(%w(http https))}\z/ | ||
|
|
||
| def items_save |
There was a problem hiding this comment.
items_save って名前だと"Items(S) save(V)"で次に目的語が期待されるのであまり正しくないですね。
update_items くらいでいいかな
There was a problem hiding this comment.
命名のことを意識もしていませんでした、、、
気をつけていきます:bow:
refs #4
[WIP]の使い方が間違っていたらすいません。アドバイスを頂いたにもかかわらずなかなか機能を追加することできないので、テーブル追加&アソシエーションの段階で一旦pushしました。
(板倉さんアドバイスを更に細分化しなければ僕は頭の整理できないのではと思いつつ)
(追加内容) 『最新の情報を取得』ボタン → 情報取得 → DBに保存 → /feeds で保存した情報表示
title/description/linkをDBに保存できるよう、テーブル(Model)を作成
RSSフィードを取ってきて、1で作ったDBに保存するactionを作成(『最新の情報を取得』ボタン)
(この際、複数回actionを実行しても同じ記事が複数登録されないように注意)
記事一覧をDBに保存しているものを表示するのに変更
RSSフィード登録時に、DBに最新記事を登録する処理を追加
定期的にRSSフィードを更新する処理を追加(cron。HerokuだとSchedulerというaddonを使うことになる)