Как я могу сделать это более объектно-ориентированным? - PullRequest
Новая
       12

Как я могу сделать это более объектно-ориентированным?

0 голосов
/ 15 февраля 2011

Я рельсовый нуб, пытающийся следовать СУХИМ методам тех, кто был до меня. Я знаю, что делаю что-то не так - я просто не уверен, что это такое или как это преодолеть.

По сути, мой вопрос: как я могу сделать этот код более объектно-ориентированным?

У меня есть класс Podcast, который на данный момент содержит кучу методов класса, которые очищают различные данные из Интернета.

Так, например, этот метод класса пытается обнаружить твиттер подкастов в Твиттере или на фейсбуке с их сайта: 

def self.social_discovery(options = {})
  new_podcasts_only = options[:new_podcasts_only] || false
  if new_podcasts_only
    podcast = Podcast.find(:all, :select => 'id, siteurl, name', :conditions => ['created_at > ? and siteurl IS NOT ?', Time.now - 24.hours, nil])
    Podcast.podcast_logger.info("#{podcast.count}")
  else
    podcast = Podcast.find(:all, :select => 'id, siteurl, name', :conditions => ['siteurl IS NOT ?', nil])
  end

  podcast.each do | pod |
    puts "#{pod.name}"
    begin 
      # Make sure the url is readable by open-uri
      if pod.siteurl.include? 'http://'
        pod_site = pod.siteurl
      else
        pod_site = pod.siteurl.insert 0, "http://"
      end

      # Skip all of this if we're dealing with a feed
      unless pod_site.downcase =~ /.rss|.xml|libsyn/i
        pod_doc = Nokogiri.HTML(open(pod_site))
        pod_name_fragment = pod.name.split(" ")[0].to_s
        if pod_name_fragment.downcase == "the"
          pod_name_fragment = pod.name.split(" ")[1].to_s unless pod.name.split(" ")[1].to_s.nil?
        end
        doc_links = pod_doc.css('a')

        # If a social url contains part of the podcast name, grab that
        # If not, grab the first one you find within our conditions
        # Give Nokogiri some room to breathe with pessimistic exception handling
        begin
          begin         
            twitter_url = doc_links.find {|link| link['href'] =~ /twitter.com\// and link['href'].match(/#{pod_name_fragment}/i).to_s != "" unless link['href'] =~ /share|status/i}.attribute('href').to_s 
          rescue Exception => ex
            if doc_links.find {|link| link['href'] =~ /twitter.com\// unless link['href'] =~ /share|status/i}.nil?
              twitter_url = nil
            else       
              twitter_url = doc_links.find {|link| link['href'] =~ /twitter.com\// unless link['href'] =~ /share|status/i}.attribute('href').to_s
            end
          end

          begin    
            facebook_url = doc_links.find {|link| link['href'] =~ /facebook.com\// and link['href'].match(/#{pod_name_fragment}/i).to_s != "" unless link['href'] =~ /share|.event/i}.attribute('href').to_s
          rescue Exception => ex
            if doc_links.find {|link| link['href'] =~ /facebook.com\// unless link['href'] =~ /share|.event/i}.nil?
              facebook_url = nil
            else       
              facebook_url = doc_links.find {|link| link['href'] =~ /facebook.com\// unless link['href'] =~ /share|.event/i}.attribute('href').to_s
            end
          end
        rescue Exception => ex
          puts "ANTISOCIAL"
        # Ensure that the urls gets saved regardless of what else happens
        ensure
          pod.update_attributes(:twitter => twitter_url, :facebook => facebook_url)            
        end

        puts "#{twitter_url}" + "#{facebook_url}"
        Podcast.podcast_logger.info("#{twitter_url}" + "#{facebook_url}")
      end
    rescue Exception => ex
      puts "FINAL EXCEPTION: #{ex.class} + #{ex.message}"
    end
  end  
end

Опять же, я знаю, что это плохой код. Пожалуйста, помогите мне понять почему ? Я буду навсегда в вашем долгу.

Спасибо

Harris

1 Ответ

2 голосов
/ 15 февраля 2011

Главное, что я вижу в вашем коде, - это дублирование кода.Если вы внимательно посмотрите, код для получения URL-адреса в твиттере и код для URL-адреса в Facebook практически одинаков, за исключением частей 'twitter.com' и 'facebook.com'.Я предлагаю вынести это в отдельный метод, который принимает переменную doc_links в качестве параметра, а также регулярное выражение для поиска ссылки.Кроме того, я не совсем уверен, почему вы выполняете часть «если ...» здесь: 

if pod_name_fragment.downcase == "the"
  pod_name_fragment = pod.name.split(" ")[1].to_s unless pod.name.split(" ")[1].to_s.nil?
end

Если вы не выполняете часть «если ...»,pod_name_fragment будет определен, но nil, но если вы его не включите, вы получите исключение, если попытаетесь сослаться на pod_name_fragment.

Кроме того, вы почти никогда не должны спасать Exception.Вместо этого используйте StandardError.Допустим, ваша программа работает, и вы пытаетесь отменить ее с помощью Ctrl-C.Это выдает исключение SystemExit (я не уверен на 100% в названии), которое является подклассом выхода Exception.В большинстве случаев вы захотите выйти сразу.Я знаю, что это не очень применимо для приложения на Rails, но я уверен, что есть и другие причины, чтобы вместо этого ловить SystemError.

Может быть и больше, один простой способ найти «плохой код» - этопосмотрите на метрики.Райан Бейтс (Ryan Bates) сделал отличный Railscast для метрик (http://railscasts.com/episodes/252-metrics-metrics-metrics),, и я бы посоветовал вам взглянуть на Рика, в частности, чтобы найти «запахи кода». Проверьте их wiki для получения информации о том, что означают разные вещи.

...