2
\$\begingroup\$

I wrote a class to import a large XML file. It works great, but I feel like its ugly, and horribly inefficient although I do like how easy it is to follow what it does.

How can I improve this to make it better and to foster some better ruby writing ability?

require "rexml/document"
class Xmltube  
  def self.convert_save(xml_data)
    doc = REXML::Document.new xml_data.read                        
    doc.elements.each("Meeting") do |meeting_element| 
      meeting = save_meeting(meeting_element)
      save_races(meeting_element, meeting)
      Rails.logger.info "all done"      
    end
  end

  def self.save_races(meeting_element,  meeting)

    meeting_element.elements.each("Races/Race") do |race_element|      
      race = save_race(race_element, meeting) 
      save_race_entrants(race_element, race)                        
    end
  end

  def self.save_race_entrants(race_element, race)
    race_element.elements.each("RaceEntries/RaceEntry") do |entry_element|
      horse = save_horse(entry_element)
      jockey = save_jockey(entry_element)
      start = save_start(entry_element, horse, jockey, race) 
      save_sumaries(entry_element, start)
    end   
  end 

  def self.save_track(meeting_element)
    # there is only one track, but still, each? wtf.
    t = {}
    meeting_element.elements.each("Track") do |track|        
      t = {
        :name       =>  track.attributes["VenueName"],
        :track_code =>  track.attributes["VenueCode"],          
        :condition  =>  track.elements['TrackRating'].text,
        :club_id    =>  save_club(meeting_element.elements["Club"]).id
      }         
    end    

    track = Track.where(:track_code =>  t[:track_code]  ).first  
    if track
      Track.update(track.id, t)    
    else      
      Track.create(t)        
    end    
  end

  def self.save_meeting meeting_element
    t = {
      :meet_code  =>  meeting_element.attributes['MeetCode'],          
      :stage      =>  meeting_element.elements["MeetingStage"].text,          
      :phase      =>  meeting_element.elements["MeetingPhase"].text,          
      :nominations_close_at =>  meeting_element.elements["NominationsClose"].text, 
      :acceptance_close_at =>   meeting_element.elements["AcceptanceClose"].text,           
      :riders_close_at      =>  meeting_element.elements["RidersClose"].text,           
      :weights_published_at =>  meeting_element.elements["WeightsPublishing"].text,          
      :club_id =>  save_club(meeting_element.elements["Club"]).id ,
      :track_id => save_track(meeting_element).id,
      :tab_status =>  meeting_element.elements["TabStatus"].text, 
      :state =>  meeting_element.elements["StateDesc"].text, 
      :day_night =>  meeting_element.elements["DayNight"].text, 
      :number_of_races =>  meeting_element.elements["NumOfRaces"].text, 
      :meet_date =>  meeting_element.elements["MeetDate"].text, 
    }         


    meeting = Meeting.where(:meet_code =>  t[:meet_code]  ).first  
    if meeting
      Meeting.update(meeting.id, t)    
    else      
      Meeting.create(t)        
    end    
  end

  ############################################################

  def self.save_sumaries entry_element, start  
    entry_element.elements.each('Form/ResultsSummaries/ResultsSummary') do | element |
      s = {
        :name             =>  element.attributes['Name'],
        :start_id         =>  start.id,
        :starts           =>  element.attributes['Starts'],
        :wins             =>  element.attributes['Wins'],
        :seconds          =>  element.attributes['Seconds'],
        :thirds           =>  element.attributes['Thirds'],
        :prize_money      =>  element.attributes['PrizeMoney'],        
      }

      sum = Summary.where(:name =>  s[:name] ).where(:start_id => s[:start_id]).first  
      if sum
        Summary.update(sum.id, s)    
      else      
        Summary.create(s)        
      end 
    end     
  end

  def self.save_start entry_element, horse, jockey, race
    s = {
      :horse_id    =>  horse.id,
      :jockey_id   =>  jockey.id,
      :race_id     =>  race.id,
      :silk        =>  entry_element.elements["JockeySilksImage"].attributes["FileName_NoExt"], 
      :start_code  =>  entry_element.attributes['RaceEntryCode'],
      :handicap_weight =>  entry_element.elements['HandicapWeight'].text, 
    }
   # Rails.logger.info entry_element['HandicapWeight'].text
    start = Start.where(:start_code =>  s[:start_code]  ).first  
    if start
      Start.update(start.id, s)    
    else      
      Start.create(s)        
    end  
  end

  def self.save_jockey entry_element
    j={
      :name         =>  entry_element.elements['JockeyRaceEntry/Name'].text,
      :jockey_code  =>  entry_element.elements['JockeyRaceEntry'].attributes["JockeyCode"],
    }
    jockey = Jockey.where(:jockey_code =>  j[:jockey_code]  ).first  
    if jockey
      Jockey.update(jockey.id, j)    
    else      
      Jockey.create(j)        
    end      
  end

  def self.save_horse entry_element
    trainer = save_trainer entry_element
    h= {
      :name       =>  entry_element.elements['Horse'].attributes["HorseName"],          
      :color      =>  entry_element.elements['Horse'].attributes["Colour"],          
      :dob        =>  entry_element.elements['Horse'].attributes["FoalDate"],           
      :sex        =>  entry_element.elements['Horse'].attributes["Sex"],           
      :trainer_id =>  trainer.id,
      :horse_code =>  entry_element.elements['Horse'].attributes["HorseCode"],                 
    }
    horse = Horse.where(:horse_code =>  h[:horse_code]  ).first  
    if horse
      Horse.update(horse.id, h)    
    else      
      Horse.create(h)        
    end        
  end

  def self.save_trainer entry_element
    t= {
        :name         =>  entry_element.elements['Trainer/Name'].text,
        :trainer_code =>  entry_element.elements['Trainer'].attributes["TrainerCode"]  
      }
    trainer = Trainer.where(:trainer_code =>  t[:trainer_code]  ).first  
    if trainer
      Trainer.update(trainer.id, t)    
    else      
      Trainer.create(t)        
    end    
  end  

  def self.save_club element
    t = {}      
      t = {
        :club_code  =>  element.attributes['ClubCode'],          
        :title      =>  element.attributes["Title"],          
      }             
    club = Club.where(:club_code =>  t[:club_code]  ).first  
    if club
      Club.update(club.id, t)    
    else      
      Club.create(t)        
    end    
  end

  def self.save_race element, meeting
    r = {
        :name       =>  element.elements['NameRaceFull'].text,
        :occur      =>  element.elements['RaceStartTime'].attributes["TimeAtVenue"],          
        :distance   =>  element.elements['RaceDistance'].text,
        :race_type  =>  element.elements['RaceType'].text,
        :track_id   =>  meeting.track_id,
        :race_code  =>  element.attributes["RaceCode"],
        :meeting_id => meeting.id       
      }        
    race = Race.where(:race_code =>  r[:race_code]  ).first  
    if race
      Race.update(race.id, r)    
    else      
      Race.create(r)        
    end  
  end

end 
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Some notes:

  • rexml/document. I'd use Nokogiri without a second thought, but if you prefer rexml because it's comes in the standard library, that's ok.
  • Rails.logger.info "all done". I wouldn't mix code with and without parens. I definetely would write them on method signatures.
  • def self.save_races. You may not need it right now, but as a rule of thumb methods return values. Here the saved races (since the method is called save_races).
  • entry_element.elements['Horse'].attributes used a lot of times: set a local variable.
  • t = {} followed by t = something inside a each? That's not good practice, should be written in a different way. If those each just want to take one element, put a first or something.
  • The most important point: almost all those save_xyz methods contain this pattern:

    sum = Summary.where(:name =>  s[:name] ).where(:start_id => s[:start_id]).first
    if sum
      Summary.update(sum.id, s)    
    else      
      Summary.create(s)        
    end 
    

    That should be abstracted with a method (in the app, or monkeypatched). You should be able to write something like (or similar, you get the idea):

    Summary.create_or_update(attributes, :find => [:name, :start_id])
    
\$\endgroup\$
1
  • \$\begingroup\$ Thanks. Iv'e applied those learnings and its looking a bit better. Most of all, its much quicker with nokogiri. I had been reluctant to use it because it causes so many installation problems, but time to be brave : ) \$\endgroup\$
    – Will
    Commented Aug 10, 2013 at 21:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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