Skip to main content
added 37 characters in body
Source Link
tokland
  • 11.2k
  • 1
  • 21
  • 26

Some notes:

  • rexml/document. I'd use Nokogiri without doubta 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 is good that functions/methodmethods return values. Here the races saved races (since the method is called save_races).

  • entry_element.elements['Horse'].attributes used a lot of times: useset 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 so you canwith 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(attrsattributes, :on_findfind => [:name, :start_id])
    

    By the way that's not a fancy abstraction I came up with here, I use it in my projects (since find_or_create_by methods from Rails fall short).

Some notes:

  • rexml/document. I'd use Nokogiri without doubt, but if you prefer rexml because it's in the standard library that's ok.

  • Rails.logger.info "all done". I wouldn't mix code with and without parens.

  • def self.save_races. You may not need it now, but as a rule of thumb is good that functions/method return values. Here the races saved (since the method is called save_races).

  • entry_element.elements['Horse'].attributes used a lot of times: use a local variable.

  • t = {} followed by t = something inside a each? That's not good practice, should be written in a different way.

  • 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 so you can write something like (or similar, you get the idea):

      Summary.create_or_update(attrs, :on_find => [:name, :start_id])
    

    By the way that's not a fancy abstraction I came up with here, I use it in my projects (since find_or_create_by methods from Rails fall short).

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])
    
added 9 characters in body
Source Link
tokland
  • 11.2k
  • 1
  • 21
  • 26

Some notes:

  • rexml/document. I'd use Nokogiri without doubt, but if you prefer rexml because it's in the standard library that's ok.

  • Rails.logger.info "all done". I wouldn't mix code with and without parens.

  • def self.save_races. You may not need it now, but as a rule of thumb is good that functions/method return values. Here the races saved (since the method is called save_races).

  • entry_element.elements['Horse'].attributes used a lot of times: use a local variable.

  • t = {} followed by t = something inside a each? That's not good practice, should be written in a different way.

  • 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 so you can write something like (or similar, you get the idea):

      Summary.create_or_update_bycreate_or_update(attrs, :on_find => [:name, :start_id])
    

    By the way that's not a fancy abstraction I came up with here, I use it in my projects (since find_or_create_by methods from Rails fall short).

Some notes:

  • rexml/document. I'd use Nokogiri without doubt, but if you prefer rexml because it's in the standard library that's ok.

  • Rails.logger.info "all done". I wouldn't mix code with and without parens.

  • def self.save_races. You may not need it now, but as a rule of thumb is good that functions/method return values. Here the races saved (since the method is called save_races).

  • entry_element.elements['Horse'].attributes used a lot of times: use a local variable.

  • t = {} followed by t = something inside a each? That's not good practice, should be written in a different way.

  • 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 so you can write something like (or similar, you get the idea):

      Summary.create_or_update_by(attrs, [:name, :start_id])
    

    By the way that's not a fancy abstraction I came up with here, I use it in my projects (since find_or_create_by methods from Rails fall short).

Some notes:

  • rexml/document. I'd use Nokogiri without doubt, but if you prefer rexml because it's in the standard library that's ok.

  • Rails.logger.info "all done". I wouldn't mix code with and without parens.

  • def self.save_races. You may not need it now, but as a rule of thumb is good that functions/method return values. Here the races saved (since the method is called save_races).

  • entry_element.elements['Horse'].attributes used a lot of times: use a local variable.

  • t = {} followed by t = something inside a each? That's not good practice, should be written in a different way.

  • 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 so you can write something like (or similar, you get the idea):

      Summary.create_or_update(attrs, :on_find => [:name, :start_id])
    

    By the way that's not a fancy abstraction I came up with here, I use it in my projects (since find_or_create_by methods from Rails fall short).

Source Link
tokland
  • 11.2k
  • 1
  • 21
  • 26

Some notes:

  • rexml/document. I'd use Nokogiri without doubt, but if you prefer rexml because it's in the standard library that's ok.

  • Rails.logger.info "all done". I wouldn't mix code with and without parens.

  • def self.save_races. You may not need it now, but as a rule of thumb is good that functions/method return values. Here the races saved (since the method is called save_races).

  • entry_element.elements['Horse'].attributes used a lot of times: use a local variable.

  • t = {} followed by t = something inside a each? That's not good practice, should be written in a different way.

  • 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 so you can write something like (or similar, you get the idea):

      Summary.create_or_update_by(attrs, [:name, :start_id])
    

    By the way that's not a fancy abstraction I came up with here, I use it in my projects (since find_or_create_by methods from Rails fall short).