CODEWITHSUNDEEP
ruby-on-railsrubyrefactoring
Marcos Placona
Marcos Placona

Reputation: 21720

Quick help refactoring Ruby Class

I've written this class that returns feed updates, but am thinking it can be further improved. It's not glitchy or anything, but as a new ruby developer, I reckon it's always good to improve :-)

class FeedManager
  attr_accessor :feed_object, :update, :new_entries

  require 'feedtosis'

  def initialize(feed_url)
    @feed_object = Feedtosis::Client.new(feed_url)
    fetch
  end

  def fetch
    @feed_object.fetch
  end

  def update
    @updates = fetch
  end

  def updated?
    @updates.new_entries.count > 0 ? true : false
  end

  def new_entries
    @updates.new_entries
  end
end

As you can see, it's quite simple, but the things I'm seeing that aren't quite right are:

  1. Whenever I call fetch via terminal, it prints a list with the updates, when it's really supposed return an object.

So as an example, in the terminal if I do something like:

client = Feedtosis::Client.new('http://stackoverflow.com/feeds')
result = client.fetch

I then get:

<Curl::Easy http://stackoverflow.com/feeds>

Which is exactly what I'd expect. However, when doing the same thing with "inniting" class with:

FeedManager.new("http://stackoverflow.com/feeds")

I'm getting the object returning as an array with all the items on the feed.

Sure I'm doing something wrong, so any help refactoring this class will he greatly appreciated.

Also, I'd like to see comments about my implementation, as well as any sort of comment to make it better would be welcome.

Thanks in advance

Upvotes: 4

Views: 230

Answers (3)

Harish Shetty
Harish Shetty

Reputation: 64363

Try this:

class FeedManager

  require 'feedtosis'

  attr_accessor :feed_object    

  def initialize(feed_url)
    self.feed_object = Feedtosis::Client.new(feed_url)
  end    
  def fetch
    feed_object.fetch
  end    
  def updates (reload = true)
    @updates = reload ? fetch : @updates
  end    
  def updated?
    updates(false).new_entries.count > 0
  end    
  def new_entries
    updates(false).new_entries
  end
end

Now you can get the updates as follows:

result = FeedManager.new("http://stackoverflow.com/feeds").updates

PS: I have removed the attr_accessor for :update, and :new_entries.

Edit

I have added code to enable conditional cache reload.

feed = FeedManager.new("http://stackoverflow.com/feeds")
updates = feed.updates # reloads the updates
# do something

updates = feed.updates(false) # get the updates from cache.

Upvotes: 1

Mike Cargal
Mike Cargal

Reputation: 6785

It appears that you expect the initialize method to return the result of calling update. Initialize is basically a constructor in Ruby, so it will return the new FeedManager object.

It's also very "unusual" to place a require statement in the middle of a class definition.

Upvotes: 0

wilhelmtell
wilhelmtell

Reputation: 58677

  1. :update, @updates

  2. count > 0 ? true : false can be just count > 0

Upvotes: 0

Related Questions