質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

新規登録して質問してみよう
ただいま回答率
85.50%
Ruby on Rails 5

Ruby on Rails 5は、オープンソースのWebアプリケーションフレームワークです。「同じことを繰り返さない」というRailsの基本理念のもと、他のフレームワークより少ないコードで簡単に開発できるよう設計されています。

Ruby

Rubyはプログラミング言語のひとつで、オープンソース、オブジェクト指向のプログラミング開発に対応しています。

オブジェクト指向

オブジェクト指向プログラミング(Object-oriented programming;OOP)は「オブジェクト」を使用するプログラミングの概念です。オブジェクト指向プログラムは、カプセル化(情報隠蔽)とポリモーフィズム(多態性)で構成されています。

リファクタリング

リファクタリングとはコードの本体を再構築するための手法であり、外見を変更せずに内部構造を変更/改善させることを指します。

Q&A

解決済

2回答

1271閲覧

Modelファイルのリファクタリングについて

Tsuyoponpon

総合スコア33

Ruby on Rails 5

Ruby on Rails 5は、オープンソースのWebアプリケーションフレームワークです。「同じことを繰り返さない」というRailsの基本理念のもと、他のフレームワークより少ないコードで簡単に開発できるよう設計されています。

Ruby

Rubyはプログラミング言語のひとつで、オープンソース、オブジェクト指向のプログラミング開発に対応しています。

オブジェクト指向

オブジェクト指向プログラミング(Object-oriented programming;OOP)は「オブジェクト」を使用するプログラミングの概念です。オブジェクト指向プログラムは、カプセル化(情報隠蔽)とポリモーフィズム(多態性)で構成されています。

リファクタリング

リファクタリングとはコードの本体を再構築するための手法であり、外見を変更せずに内部構造を変更/改善させることを指します。

0グッド

0クリップ

投稿2019/09/25 07:00

現在リファクタリングを勉強しているのですが、なかなか思う様に動かず学習がストップしています。特に悩んでいるのは、どこの処理をまとめてメソッド化するのかということです。オブジェクト指向やリーダブルコードの本を一式読んで大体のイメージはあるのですが、自分のコードに置き換えるとたちまちエラーで進みません。自分では全くリファクタリングの見当がつかず、少し丸投げの様な質問になっています。以下には自分なりに試したことやわからないことを記載しますので、ぜひご教授いただければと思います。

リファクタリング対象のコード

ruby

1#app/models/clone.rb 2class Clone < ApplicationRecord 3 4 require 'nokogiri' 5 require 'httparty' 6 require 'byebug' 7 8 url = 'https://terateil' 9 unparsed_page = HTTParty.get(url) 10 parsed_page = Nokogiri::HTML(unparsed_page) 11 12 vol_listing = parsed_page.css('div.card-eventSingle') 13 page = 1 14 card_body_count = vol_listing.count 15 total_before = parsed_page.css('strong')[1].text 16 total = total_before.to_i 17 last_page = (total.to_f / card_body_count.to_f).round 18 while page <= last_page 19 pagination_url = "https://terateil#{page}" 20 pagination_unparsed_page = HTTParty.get(pagination_url) 21 pagination_parsed_page = Nokogiri::HTML(pagination_unparsed_page) 22 pagination_vol_listing = pagination_parsed_page.css('div.card-eventSingle') 23 pagination_vol_listing.each do |vol| 24 volunteer = { 25 title: vol.css('a.two-four-lines').text , 26 group: vol.css('a.d-inline-block').text, 27 content: vol.css('p.two-lines').text, 28 url: 'https://terateil' + vol.css('a.two-four-lines')[0].attributes["href"].value 29 } 30 url = volunteer[:url] 31 unparsed_page = HTTParty.get(url) 32 parsed_page = Nokogiri::HTML(unparsed_page) 33 db_table = Clone.find_by(url: url) 34 35 if db_table.nil? 36 title = volunteer[:title] 37 group = volunteer[:group] 38 content = volunteer[:content] 39 scrape = Clone.new(title: title, group: group, content: content, url: url) 40 scrape.save 41 end 42 end 43 page += 1 44 end 45

試したこと

ruby

1class Clone < ApplicationRecord 2 3 def self.set_url_for_scraping(set_url) 4 url = 'seturl' 5 unparsed_page = HTTParty.get(url) 6 parsed_page = Nokogiri::HTML(unparsed_page) 7 end 8 9 require 'nokogiri' 10 require 'httparty' 11 require 'byebug' 12 13 vol_listing = parsed_page.css('div.card-eventSingle') 14 page = 1 15 card_body_count = vol_listing.count 16 total_before = parsed_page.css('strong')[1].text 17 total = total_before.to_i 18 last_page = (total.to_f / card_body_count.to_f).round 19 while page <= last_page 20 Clone.set_url_for_scraping(https://terateil#{page}) #クラスメソッド呼び出し 21 pagination_vol_listing = pagination_parsed_page.css('div.card-eventSingle') 22 pagination_vol_listing.each do |vol| 23 volunteer = { 24 title: vol.css('a.two-four-lines').text , 25 group: vol.css('a.d-inline-block').text, 26 content: vol.css('p.two-lines').text, 27 url: 'https://terateil' + vol.css('a.two-four-lines')[0].attributes["href"].value 28 } 29       self.set_url_for_scraping(volunteer[:url]) #クラスメソッド呼び出し 30 db_table = Clone.find_by(url: url) 31 32 if db_table.nil? 33 title = volunteer[:title] 34 group = volunteer[:group] 35 content = volunteer[:content] 36 scrape = Clone.new(title: title, group: group, content: content, url: url) 37 scrape.save 38 end 39 end 40 page += 1 41 end 42end

こんな感じで「set_url_for_scraping(set_url)」を作ったりしましたが、一つ作った時点でエラーが出ました。

補足(わからないこと)

Rails: 5.2.3
OS: Mac

一度答えを見ればイメージがつかめるはずなのですが、自分で書いたコードのため正解がわかりません。答えを聞く様な形になってしまいますが、このままでは全く先に進めませんので、一度正解例を見せていただき、学習を再開させていただければ幸いです。

気になる質問をクリップする

クリップした質問は、後からいつでもMYページで確認できます。

またクリップした質問に回答があった際、通知やメールを受け取ることができます。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

ozwk

2019/09/25 07:38

「どこの処理をまとめてメソッド化するのか」の筋が悪いからエラーが出てるのではなく 単に文法がおかしいからエラーが出ています。 つまりリファクタリングとかそれ以前の問題です。
Tsuyoponpon

2019/09/25 12:17

ozwk さん 回答ありがとうございます。 再度文法を勉強したいと思います! ありがとうございました!
guest

回答2

0

ベストアンサー

まず、リファクタリングの前にインデントを揃えましょう。

メソッドにまとめる前に明らかに冗長なコードを書き直しましょう(while文)。

rb

1while page <= last_page 2 pagination_url = "https://terateil#{page}" 3 pagination_unparsed_page = HTTParty.get(pagination_url) 4 pagination_parsed_page = Nokogiri::HTML(pagination_unparsed_page) 5 pagination_vol_listing = pagination_parsed_page.css('div.card-eventSingle') 6 pagination_vol_listing.each do |vol| 7 volunteer = { 8 title: vol.css('a.two-four-lines').text , 9 group: vol.css('a.d-inline-block').text, 10 content: vol.css('p.two-lines').text, 11 url: 'https://terateil' + vol.css('a.two-four-lines')[0].attributes["href"].value 12 } 13 url = volunteer[:url] 14 unparsed_page = HTTParty.get(url) 15 parsed_page = Nokogiri::HTML(unparsed_page) 16 db_table = Clone.find_by(url: url) 17 18 if db_table.nil? 19 title = volunteer[:title] 20 group = volunteer[:group] 21 content = volunteer[:content] 22 scrape = Clone.new(title: title, group: group, content: content, url: url) 23 scrape.save 24 end 25 end 26 page += 1 27end

rb

1(1..last_page).each do |n| 2 pagination_parsed_page = Nokogiri::HTML(HTTParty.get("https://terateil#{n}")) 3 pagination_parsed_page.css('div.card-eventSingle').each do |vol| 4 volunteer = { title: vol.css('a.two-four-lines').text , 5 group: vol.css('a.d-inline-block').text, 6 content: vol.css('p.two-lines').text, 7 url: "https://terateil#{vol.css('a.two-four-lines')[0].attributes["href"].value}" } 8 parsed_page = Nokogiri::HTML(HTTParty.get(volunteer[:url])) # 使われてない? 9 10 next unless Clone.find_by(url: volunteer[:url]) 11 12 Clone.create!(title: volunteer[:title], 13 group: volunteer[:group], 14 content: volunteer[:content], 15 url: volunteer[:url]) 16 end 17end

その後メソッドに分ければいいです。

rb

1(1..last_page).each do |n| 2 pagination_parsed_page = Nokogiri::HTML(HTTParty.get("https://terateil#{n}")) 3 pagination_parsed_page.css('div.card-eventSingle').each do |vol| 4 volunteer = make_volunteer(vol) 5 parsed_page = Nokogiri::HTML(HTTParty.get(volunteer[:url])) # 使われてない? 6 7 next unless Clone.find_by(url: volunteer[:url]) 8 9 Clone.create!(title: volunteer[:title], 10 group: volunteer[:group], 11 content: volunteer[:content], 12 url: volunteer[:url]) 13 end 14end 15 16def make_volunteer(vol) 17 { title: vol.css('a.two-four-lines').text , 18 group: vol.css('a.d-inline-block').text, 19 content: vol.css('p.two-lines').text, 20 url: "https://terateil#{vol.css('a.two-four-lines')[0].attributes["href"].value}" } 21end

大前提としてリファクタリングは厳密な正解があるわけではなく、読みやすいコードであることが第一です。
何でもかんでもリファクタリングするのではなく、必要な箇所だけリファクタリングましょう。

投稿2019/09/25 07:41

編集2019/09/25 07:43
Mugheart

総合スコア2340

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

Tsuyoponpon

2019/09/25 12:21

Mugheart さん ご丁寧に回答いただきまして誠にありがとうございます! なんでもかんでもリファクタリングをしなければいけないと思っていましたので、少し考え方が変わりました!ありがとうございます! 修正していただいた部分を参考にして、勉強を再開していきたいと思います! 丁寧にご指導いただき、大まかなリファクタリングのイメージを掴ませていただきました! つきましてはベストアンサーに選出させていただき、重ねてお礼申し上げます。
guest

0

一つ作った時点でエラーが出ました。

具体的なエラーの内容を確認するのが第一です。「エラーが出ないけど結果が正しくない」というようなバグのほうが、追いかけるのは厄介なものです。

一度正解例を見せていただき

一言で言えば、「正解」なんてものはありません

投稿2019/09/25 07:28

maisumakun

総合スコア145123

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

Tsuyoponpon

2019/09/25 12:16

maisumakun さん 回答ありがとうございます! 教えていただいたことを真摯に受け止め、再度勉強したいと思います!!
guest

あなたの回答

tips

太字

斜体

打ち消し線

見出し

引用テキストの挿入

コードの挿入

リンクの挿入

リストの挿入

番号リストの挿入

表の挿入

水平線の挿入

プレビュー

15分調べてもわからないことは
teratailで質問しよう!

ただいまの回答率
85.50%

質問をまとめることで
思考を整理して素早く解決

テンプレート機能で
簡単に質問をまとめる

質問する

関連した質問