Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my feed parser method using feedzirra. It works, but I feel dirty because I can't figure out how to improve this code. Any suggestions?

class Feed < ActiveRecord::Base

  attr_accessible :name, :summary, :url, :published_at, :guid

  def self.update_from_feed
    feed_urls = ["url1", "url2", "ulr3", "url4"]
    feeds = Feedzirra::Feed.fetch_and_parse(feed_urls)

    feeds.each do |url|
      feeds = url.last.entries
      feeds.each do |entry|
        logger.debug "Questo e l'oggetto #{entry}"
        unless exists? :guid => entry.entry_id
          create!(
            :name         => entry.title,
            :summary      => entry.summary,
            :url          => entry.url,
            :published_at => entry.published,
            :guid         => entry.entry_id
          )
        end
      end
    end

  end
end

This is the object returned from the fetch_and_parse method: with just the first URL

{"http://www.agichina24.it/home/agenzia-nuova-cina/rss2"=>#<Feedzirra::Parser::RSS:0x00000005db8e68 @title="rss", @url="http://www.agichina24.it/home/agenzia-nuova-cina", @entries=[#<Feedzirra::Parser::RSSEntry:0x00000005db6500 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207121646-cro-rt10178-prestiti_denominati_in_yuan_in_crescita", @entry_id="dY_9w7JofPUA7twYye5aMg", @title=" PRESTITI DENOMINATI IN YUAN IN CRESCITA", @summary="Pechino, 12 lug. ? Impennata per i nuovi prestiti denominati in\nyuan nel mese di giugno, grazie all'iniziativa del governo per\nstimolare l'economia in rallentamento.", @published=2012-07-12 14:45:17 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005dc8fe8 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207121645-cro-rt10177-pechino_stretta_su_spese_funzionari_pubblici", @entry_id="3PxPS6grfUl2G2d0NyLahA", @title=" PECHINO, STRETTA SU SPESE FUNZIONARI PUBBLICI", @summary="Pechino, 12 lug. ? Il governo municipale di Pechino rafforzerà\ni controlli sui viaggi all'estero dei funzionari per\npartecipare ad attività di formazione.", @published=2012-07-12 14:44:42 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005dc6ce8 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207091655-cro-rt10173-colata_di_fango_nel_sichuan_14_morti_confermati", @entry_id="h74sGs2DolJKaZZyZWl84g", @title=" COLATA DI FANGO NEL SICHUAN, 14 MORTI CONFERMATI", @summary="Chengdu, 9 lug.- I soccorritori hanno recuperato i corpi di 14\npersone dopo che una colata di fango, provocata dalle piogge,\naveva colpito la provincia del Sichuan lo scorso 28 giugno.", @published=2012-07-09 14:55:11 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ebb1f8 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207091655-cro-rt10171-attivisti_giapponesi_sulle_diaoyu_proteste_cinesi", @entry_id="7bh55Qqb5dcEijmhCbtLYA", @title=" ATTIVISTI GIAPPONESI SULLE DIAOYU, PROTESTE CINESI", @summary="Pechino, 9 lug.- La Cina ha presentato solenni rimostranze e\nproteste verso il Giappone per aver violato la sua sovranita'\nterritoriale, dopo che due attivisti giapponesi sono sbarcati\nsulle isole Diaoyu.", @published=2012-07-09 14:54:29 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ec0ef0 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207091654-cro-rt10170-incontro_tra_funzionari_di_cina_e_asean", @entry_id="fIrGROI7wDYX0pxsoiS9og", @title=" INCONTRO TRA FUNZIONARI DI CINA E ASEAN", @summary="Phnom Penh, 9 lug.- Domenica si sono incontrati alti funzionari\nin rappresentanza di Cina e Asean, in vista della prossima\nriunione dei ministri degli Esteri.", @published=2012-07-09 14:54:07 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ebebf0 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207061449-cro-rt10139-eclac_si_a_piu_cooperazione_cina_america_latina", @entry_id="K7sni4Vq4wnUEQvtxjKbmg", @title=" ECLAC, SI' A PIU' COOPERAZIONE CINA-AMERICA LATINA", @summary="Santiago, 6 lug. - Elogio alle recenti proposte di Wen Jiabao\nper rafforzare le relazioni di cooperazione tra Cina e America\nLatina.", @published=2012-07-06 12:48:26 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ec7048 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207061448-cro-rt10138-maxi_retata_contro_traffico_di_bambini", @entry_id="aeJgnkQBbTr6finnow2Ckw", @title=" MAXI RETATA CONTRO TRAFFICO DI BAMBINI", @summary="Pechino, 6 lug. - La polizia cinese ha sgominato lunedi' due\ngrandi bande di traffico di bambini: 802 i sospettati\narrestati, 181 i bambini liberati.", @published=2012-07-06 12:47:57 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ec4d48 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207061448-cro-rt10137-hong_kong_record_di_investimenti_esteri", @entry_id="UNxnzwr3oPFEOtES1W4YPg", @title=" HONG KONG, RECORD DI INVESTIMENTI ESTERI", @summary="Hong Kong, 6 lug. - Nel 2011 gli investimenti diretti esteri\n(Fdi) a Hong Kong hanno superato gli 83 miliardi di dollari\nstatunitensi, un record storico, per un piu' 17% rispetto al\n2010.", @published=2012-07-06 12:47:17 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ecda60 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207051516-cro-rt10164-xinjiang_esercitazioni_antiterrorismo", @entry_id="lxvg0rXeNbKGaHiiueQ2tA", @title=" XINJIANG, ESERCITAZIONI ANTITERRORISMO", @summary="Urumqi, 5 lug. - Effettuata un'esercitazione antiterrorismo da\nparte di forze speciali a Urumqi, nella regione autonoma del\nXinjiang Uygur.", @published=2012-07-05 13:15:29 UTC>, #<Feedzirra::Parser::RSSEntry:0x00000005ecb760 @url="http://www.agichina24.it/home/agenzia-nuova-cina/notizie/201207051515-cro-rt10163-raul_castro_a_pechino_per_visita_di_stato", @entry_id="kJjP4UZ_rmbMTyIBKpNRcA", @title=" RAUL CASTRO A PECHINO PER VISITA DI STATO", @summary="Pechino, 5 lug. -  Raul Castro Ruz e' giunto mercoledi' a\nPechino per una visita di Stato in Cina, su invito del\npresidente Hu Jintao.", @published=2012-07-05 13:14:25 UTC>], @feed_url="http://www.agichina24.it/home/agenzia-nuova-cina/rss2", @etag=nil, @last_modified=nil>} 

I've tried everything, but only that solution works.

share|improve this question

2 Answers 2

The variable names in your blocks are very confusing, especially where you introduce another feeds. Just go with the least surprising names possible:

feed_urls = ["url1", "url2", "ulr3", "url4"]
feeds = Feedzirra::Feed.fetch_and_parse(feed_urls)

feeds.each do |feed|
  entries = feed.last.entries
  entries.each do |entry|
    # ...
  end
end

Also, adding parentheses would help make this easier for programmers to parse:

exists?(:guid => entry.entry_id)
share|improve this answer

Slight cleanup:

class Feed < ActiveRecord::Base
  # ...

  def self.update_from_feed
    feed_urls = ["url1", "url2", "ulr3", "url4"]
    Feedzirra::Feed.fetch_and_parse(feed_urls).each do |_, feeds|
      feeds.entries.each do |entry|
        # ...
      end
    end
  end
end
  1. No need to store a local variable called feeds anymore.

  2. If you call .each on a hash, you can use |key, value| in the block.

2a. Since url isn't getting used anymore, |url, feeds| becomes |_, feeds|.

Thoughts?

share|improve this answer
    
Oops -- super necro here. Didn't realize how unpopulated codereview.stackexchange was... –  Kyle Mar 14 '13 at 22:21
    
Sorry for the late comment, but we're working on that, Kyle :) I stumbled on your answer and I think it looks good. I hope you will stick around here more. –  Simon André Forsberg Dec 8 '13 at 18:22

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.