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

Question

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

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

#app/models/clone.rb
class Clone < ApplicationRecord

   require 'nokogiri'
   require 'httparty'
   require 'byebug'

    url = 'https://terateil'
    unparsed_page = HTTParty.get(url)
    parsed_page = Nokogiri::HTML(unparsed_page)

    vol_listing = parsed_page.css('div.card-eventSingle') 
    page = 1
    card_body_count = vol_listing.count 
    total_before = parsed_page.css('strong')[1].text
    total = total_before.to_i  
    last_page = (total.to_f / card_body_count.to_f).round 
    while page <= last_page
        pagination_url = "https://terateil#{page}"
        pagination_unparsed_page = HTTParty.get(pagination_url)
        pagination_parsed_page = Nokogiri::HTML(pagination_unparsed_page)
        pagination_vol_listing = pagination_parsed_page.css('div.card-eventSingle')
        pagination_vol_listing.each do |vol|
            volunteer = {
                title: vol.css('a.two-four-lines').text ,
                group: vol.css('a.d-inline-block').text, 
                content: vol.css('p.two-lines').text, 
                url: 'https://terateil' + vol.css('a.two-four-lines')[0].attributes["href"].value
            }
            url = volunteer[:url] 
            unparsed_page = HTTParty.get(url)
            parsed_page = Nokogiri::HTML(unparsed_page)
            db_table = Clone.find_by(url: url)

            if db_table.nil?
                  title = volunteer[:title]
                  group = volunteer[:group]
                  content = volunteer[:content]
                  scrape =  Clone.new(title: title, group: group, content: content, url: url)
                  scrape.save
            end
        end
      page += 1
    end

試したこと

class Clone < ApplicationRecord

  def self.set_url_for_scraping(set_url)  
    url = 'seturl'
    unparsed_page = HTTParty.get(url)
    parsed_page = Nokogiri::HTML(unparsed_page)
  end

   require 'nokogiri'
   require 'httparty'
   require 'byebug'

    vol_listing = parsed_page.css('div.card-eventSingle') 
    page = 1
    card_body_count = vol_listing.count 
    total_before = parsed_page.css('strong')[1].text
    total = total_before.to_i  
    last_page = (total.to_f / card_body_count.to_f).round 
    while page <= last_page
        Clone.set_url_for_scraping(https://terateil#{page}) #クラスメソッド呼び出し
        pagination_vol_listing = pagination_parsed_page.css('div.card-eventSingle')
        pagination_vol_listing.each do |vol|
            volunteer = {
                title: vol.css('a.two-four-lines').text ,
                group: vol.css('a.d-inline-block').text, 
                content: vol.css('p.two-lines').text, 
                url: 'https://terateil' + vol.css('a.two-four-lines')[0].attributes["href"].value
            }
　　　　　　　self.set_url_for_scraping(volunteer[:url])　#クラスメソッド呼び出し
            db_table = Clone.find_by(url: url)

            if db_table.nil?
                  title = volunteer[:title]
                  group = volunteer[:group]
                  content = volunteer[:content]
                  scrape =  Clone.new(title: title, group: group, content: content, url: url)
                  scrape.save
            end
        end
      page += 1
    end
end

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

補足（わからないこと）

Rails: 5.2.3
OS: Mac

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

Comments

[ozwk]

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

[Tsuyoponpon]

ozwk　さん

回答ありがとうございます。
再度文法を勉強したいと思います！

ありがとうございました！

Answer 1

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

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

while page <= last_page
  pagination_url = "https://terateil#{page}"
  pagination_unparsed_page = HTTParty.get(pagination_url)
  pagination_parsed_page = Nokogiri::HTML(pagination_unparsed_page)
  pagination_vol_listing = pagination_parsed_page.css('div.card-eventSingle')
  pagination_vol_listing.each do |vol|
    volunteer = {
      title: vol.css('a.two-four-lines').text ,
      group: vol.css('a.d-inline-block').text, 
      content: vol.css('p.two-lines').text, 
      url: 'https://terateil' + vol.css('a.two-four-lines')[0].attributes["href"].value
    }
    url = volunteer[:url] 
    unparsed_page = HTTParty.get(url)
    parsed_page = Nokogiri::HTML(unparsed_page)
    db_table = Clone.find_by(url: url)

    if db_table.nil?
      title = volunteer[:title]
      group = volunteer[:group]
      content = volunteer[:content]
      scrape =  Clone.new(title: title, group: group, content: content, url: url)
      scrape.save
    end
  end
  page += 1
end

(1..last_page).each do |n|
  pagination_parsed_page = Nokogiri::HTML(HTTParty.get("https://terateil#{n}"))
  pagination_parsed_page.css('div.card-eventSingle').each do |vol|
    volunteer = { title: vol.css('a.two-four-lines').text ,
                  group: vol.css('a.d-inline-block').text, 
                  content: vol.css('p.two-lines').text, 
                  url: "https://terateil#{vol.css('a.two-four-lines')[0].attributes["href"].value}" }
    parsed_page = Nokogiri::HTML(HTTParty.get(volunteer[:url])) # 使われてない？

    next unless Clone.find_by(url: volunteer[:url])

    Clone.create!(title: volunteer[:title],
                  group: volunteer[:group],
                  content: volunteer[:content],
                  url: volunteer[:url])
  end
end

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

(1..last_page).each do |n|
  pagination_parsed_page = Nokogiri::HTML(HTTParty.get("https://terateil#{n}"))
  pagination_parsed_page.css('div.card-eventSingle').each do |vol|
    volunteer = make_volunteer(vol)
    parsed_page = Nokogiri::HTML(HTTParty.get(volunteer[:url])) # 使われてない？

    next unless Clone.find_by(url: volunteer[:url])

    Clone.create!(title: volunteer[:title],
                  group: volunteer[:group],
                  content: volunteer[:content],
                  url: volunteer[:url])
  end
end

def make_volunteer(vol)
  { title: vol.css('a.two-four-lines').text ,
    group: vol.css('a.d-inline-block').text, 
    content: vol.css('p.two-lines').text, 
    url: "https://terateil#{vol.css('a.two-four-lines')[0].attributes["href"].value}" }
end

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

Comments

[Tsuyoponpon]

Mugheart　さん

ご丁寧に回答いただきまして誠にありがとうございます！
なんでもかんでもリファクタリングをしなければいけないと思っていましたので、少し考え方が変わりました！ありがとうございます！

修正していただいた部分を参考にして、勉強を再開していきたいと思います！

丁寧にご指導いただき、大まかなリファクタリングのイメージを掴ませていただきました！
つきましてはベストアンサーに選出させていただき、重ねてお礼申し上げます。

Answer 2

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

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

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

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

Comments

[Tsuyoponpon]

maisumakun さん
回答ありがとうございます！

教えていただいたことを真摯に受け止め、再度勉強したいと思います！！