Created
February 4, 2013 20:36
-
-
Save mileszs/4709536 to your computer and use it in GitHub Desktop.
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)?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 |
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)?
Author
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.