views:

108

answers:

3

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&gt;

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

A: 

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.

Mike Cargal
Where should this be then?
Marcos Placona
You don't seem to be keeping the object you're creating. What are you trying to accomplish? Do you just want the results of creating a Feedtosis object and getting it's updates (and then tossing the object), hen you should probably just def a convenience method to your script.
Mike Cargal
I have fixed the code in the `initialize` method. Try again.
KandadaBoggu
A: 
  1. :update, @updates

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

wilhelmtell
Not sure what you mean with::update, @updates
Marcos Placona
I think he's referring to the fact that you `attr_accessor` ize `:update`, yet you are using `@updates` everywhere and not `@update`.
theIV
+1  A: 

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.
KandadaBoggu
Not sure I;m missing something here, but am i supposed to call: result = FeedManager.new("http://stackoverflow.com/feeds").updates everytime? Also, is I do:result = FeedManager.new("http://stackoverflow.com/feeds") and the tryresult.updates, I get "You have a nil object when you didn't expect it!"
Marcos Placona
Does the same URL work when you call Feedtosis::Client.new(feed_url).fetch directly?
KandadaBoggu
assign to @feed_object instead of feed_object in the initialize method
Mike Cargal
I have fixed the code in the initialize method. Try again.
KandadaBoggu
Hi, this looks great!! only on tiny thing though:I changed the updates method to do:updates ||= fetch, so it doesn't cache the variable and grabs the updates. It all seems to be working nicely, but on the new_entries method, it's calling the update method, which no longer being cached, means a new call to update will be made, therefore slowing down the process.Any way i can use updates without caching, but on new_entries use the version loaded by updates? Let me know if you need more clarification on that. but kinda seems overkill to load it twice
Marcos Placona
I have updated the code. Take a look.
KandadaBoggu
@KandadaBoggu you're a star! It worked flawlessly, and does exactly what I wanted it to do. Thank you very much!
Marcos Placona