CODEWITHSUNDEEP
ruby-on-railsooparchitecturerefactoringclass-method
lightyrs
lightyrs

Reputation: 2839

How can I make this more object-oriented?

I'm a rails noob trying to follow the DRY methods of those who've come before me. I know that I'm doing something wrong – I'm just not sure what that is or how to overcome it.

Basically, my question is how can I make this code more object-oriented?

I have a class Podcast, which at this point just contains a bunch of class methods that scrape various data from around the web.

So, for example, this class method tries to discover a podcast twitter or facebook feed from their website:

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

Again, I know this is bad code. Please help me understand why? I will be forever in your debt.

Thanks,

Harris

Upvotes: 0

Views: 148

Answers (1)

sarahhodne
sarahhodne

Reputation: 10106

The major thing I can see in your code is code duplication. If you look closely, the code for fetching the twitter url and the code for the facebook url is almost exactly the same, except for the 'twitter.com' and 'facebook.com' part. My suggestion is to pull that out into a separate method that takes the doc_links variable as a parameter as well as the regex for finding the link. Also, I'm not quite sure why you're doing the "unless ..." part here:

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

If you don't do the "unless ..." part of the line, pod_name_fragment will be defined but nil, but if you don't include it you will get an exception if you try to refer to pod_name_fragment.

Also, you should almost never rescue Exception. Use StandardError instead. Let's say your program is running and you try to cancel it with Ctrl-C. That throws a SystemExit (I'm not 100% sure about the name) exception, which is a subclass of the Exception exit. In most cases you would then want to exit right away. I know this isn't that much applicable for a Rails app, but I'm pretty sure there's other reasons to catch SystemError instead.

There might be more, one easy way to find "bad code" is to look at metrics. Ryan Bates made an excellent Railscast on metrics (http://railscasts.com/episodes/252-metrics-metrics-metrics), and I'd suggest you look at Reek in particular for finding "code smells". Check their wiki for information on what the different things mean.

Upvotes: 2

Related Questions