Each document instance has categories and pages that are generated from "master categories" and "master pages". I think a refactor is in order. I would like to compare my result with the thoughts of others. How would you refactor this code (if you would refactor it)?

category.rb

class Category < ActiveRecord::Base
  belongs_to :document
  belongs_to :master_category

  has_many :pages

  def generate_pages!
    if pages.empty?
      master_category.master_pages.each do |master_page|
        pages.create!(master_page.page_attributes)
      end
    end
    self.save!
  end

end

document.rb

class Document < ActiveRecord::Base
  has_many :categories
  has_many :master_categories, through: :categories
  has_many :pages, through: :categories

  def generate_categories!
    MasterCategory.all.each do |master_category|
      self.categories.create!(master_category.category_attributes)
    end
  end

  def generate_pages!
    self.categories.each do |category|
      category.generate_pages!
    end
  end

end

master_category.rb

class MasterCategory < ActiveRecord::Base
  has_many :categories
  has_many :master_pages

  def category_attributes
    self.attributes.slice('position', 'name').merge(master_category_id: self.id)
  end
end

master_page.rb

class MasterPage < ActiveRecord::Base
  belongs_to :master_category
  has_many :pages

  def page_attributes
    self.attributes.slice('position', 'name', 'index_page').merge(master_page_id: self.id)
  end
end

How is this code actually getting called? Internally, it looks fairly clean. Not sure I would do much if anything to refactor at the moment.

Also, are generate_categories! and generate_pages! just called once upon document creation, or getting called more than once?

Minor change -- if you plan to have a lot of MasterCategories at some point, you might want to do categories.build and save at the end, rather than doing a create on each iteration.

Same with generate_pages!, and I might move the save inside the if. That way, if you've made other changes to the object, you need to deal with those independently rather than relying on the accidental behavior that generate_pages! saves even if no pages change.

Needs more transaction blocks wrapped around those each loops. Are these generate methods called called over and over and need to be idempotent (first_or_create)?

Thanks, guys!

They are called once per document, in the controller, during a wizard-esque process for creating a document.

I ended up making CategoriesGenerator and PagesGenerator classes that do all of the work. I'll post those soon, with some explanation.