Geek Answers Handbook

How can I improve this Rails code?

I am writing a small browser game as a project to learn RoR, and I am completely new to this.

This is a small method that is regularly called cronjob.

I suppose there must be some way to add elements to the potions array and then do a bulk save at the end, I also don't like hitting db every time in the loop to get the number of elements for the market again.

def self.restock_energy_potions market = find_or_create_market potions = EnergyPotion.find_all_by_user_id(market.id) while (potions.size < 5) potion = EnergyPotion.new(:user_id => market.id) potion.save potions = EnergyPotion.find_all_by_user_id(market.id) end end
+4
source share
3 answers

I'm not sure I understand your question. Are you looking for something like this?

 def self.restock_energy_potions market = find_or_create_market potions = EnergyPotion.find_all_by_user_id(market.id) (potions.size...5).each {EnergyPotion.new(:user_id => market.id).save } end end

Note the triple points in the range; You do not want to create a potion if there are already 5 of them.

In addition, if your potions were linked (for example, has_many ), you could create them through the market.potions property (I assume here, about the relationship between users and markets), the details depend on how your models are set up) and save them all at once. I do not think that saving on the database would be significant.

+8
source

Assuming your potions for the market / user has_many , you can do this:

 def self.restock_energy_potions market = find_or_create_market (market.potions.size..5).each {market.potions.create(:user_id => market.id)} end
0
source

a) use associations:

 class Market < AR::Base # * note that if you are not dealing with a legacy schema, you should # rename user_id to market_id and remove the foreigh_key assignment. # * dependent => :destroy is important or you'll have orphaned records # in your database if you ever decide to delete some market has_many :energy_potions, :foreign_key => :user_id, :dependent => :destroy end class EnergyPotion < AR::Base belongs_to :market, :foreign_key => :user_id end

b) there is no need to reload the association after adding each of them. also move the functionality to the model:

 find_or_create_market.restock class Market def restock # * note 4, not 5 here. it starts with 0 (market.energy_potions.size..4).each {market.energy_potions.create!} end end

c) also pay attention to the creation! and do not create. You must detect errors. error handling is application dependent. in your case, since you start it from cron, you can do several things * send an email with a warning * catch exceptions and register them, (exception_notifier plugin or hosting hosting) * print to stderror and configure cron to send errors to some electronic letters.

  def self.restock_potions market = find_or_create market.restock rescue ActiveRecord::RecordInvalid ... rescue ... end
0
source

More articles:


All Articles