How can I improve this Rails code?

Question

I'm writing a little browser game as a project to learn RoR with and I'm quite new to it.

This is a little method that's called regularly by a cronjob.

I'm guessing there should be some way of adding elements to the potions array and then doing a bulk save at the end, I'm also not liking hitting the db each time in the loop to get the number of items 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

Answer 1

I'm not sure I'm understanding 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 dots in the range; you don't want to create a potion if there are already 5.

Also, if your potions were linked (e.g. by has_many) you could create them through the market.potions property (I'm guessing here, about the relationship between users and markets--details depend on how your models are set up) and save them all at once. I don't think the data base savings would be significant though.

Answer 2

Assuming that your market/user has_many potions, 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

Answer 3

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) no need to reload the association after adding each one. also move the functionality into 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 note create! and not create. you should detect errors. error handling depends on the application. in your case since you run it from cron you can do few things * send email with alert * catch exceptions and log them, (exception_notifier plugin, or hoptoad hosted service) * print to stderror and configuring cron to send errors to some email.

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