New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factories simple to ridiculous #15

Merged
merged 12 commits into from Apr 1, 2016

Conversation

Projects
None yet
3 participants
@IanWhitney
Owner

IanWhitney commented Feb 4, 2016

@dreyks

This comment has been minimized.

Show comment
Hide comment
@dreyks

dreyks Feb 4, 2016

Campus.subclasses.detect(->{UnknownCampus.new}) { ...skip... }.new

should be ->{UnknownCampus} without new. Otherwise You'll get new called twice

dreyks commented Feb 4, 2016

Campus.subclasses.detect(->{UnknownCampus.new}) { ...skip... }.new

should be ->{UnknownCampus} without new. Otherwise You'll get new called twice

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Feb 4, 2016

Owner

@dreyks Thanks for spotting that! Fixing it now.

Owner

IanWhitney commented Feb 4, 2016

@dreyks Thanks for spotting that! Fixing it now.

@roryokane

This comment has been minimized.

Show comment
Hide comment
@roryokane

roryokane Feb 5, 2016

Contributor

More comments are on this story’s post on Lobsters.

Contributor

roryokane commented Feb 5, 2016

More comments are on this story’s post on Lobsters.

roryokane added some commits Feb 5, 2016

Change `def…def` to `def…end` in factories post
This fixes syntax errors in the code that prevented it from running.
Fix `handles?` in factory post by changing = to ==
`=` is assignment, `==` is testing equality.
@roryokane

This comment has been minimized.

Show comment
Hide comment
@roryokane

roryokane Feb 5, 2016

Contributor

Here is another solution – creating a class AbbreviatedCampuses that all campuses register themselves with using AbbreviatedCampuses.register self. All campuses also implement an abbreviation method as the single place their abbreviation is written.

With this method, when you add a new campus, you only need to change one place in the code. But you don’t have to do the slow and hard-to-understand metaprogramming with ObjectSpace or Object.const_get – you explicitly link the new campus to the new registry. And the registration line in each campus class is shorter than the three-line self.campus_like? definition with the ObjectSpace solution.

class UnknownCampus
  def name
    "Unknown Campus"
  end

  def mascot
    "Unknown Mascot"
  end
end

class AbbreviatedCampuses
  @@campus_like_classes = Hash.new(UnknownCampus)

  def self.register(klass)
    abbreviation = klass.new.abbreviation
    @@campus_like_classes[abbreviation] = klass
  end

  def self.build_from(abbreviation)
    @@campus_like_classes[abbreviation].new
  end
end

# The above two classes must be loaded first.
# The rest can load in any order.

class CampusDetails
  def campus_name(abbreviation)
    AbbreviatedCampuses.build_from(abbreviation).name
  end

  def campus_mascot(abbreviation)
    AbbreviatedCampuses.build_from(abbreviation).mascot
  end
end

class UMNTC
  def name
    "University of Minnesota Twin Cities"
  end
  def mascot
    "Gopher"
  end
  def abbreviation
    "UMNTC"
  end

  AbbreviatedCampuses.register self
end

class UMNMO
  def name
    "University of Minnesota Morris"
  end
  def mascot
    "Cougar"
  end
  def abbreviation
    "UMNMO"
  end

  AbbreviatedCampuses.register self
end

class UMNCR
  def name
    "University of Minnesota Crookston"
  end
  def mascot
    "Golden Eagle"
  end
  def abbreviation
    "UMNCR"
  end

  AbbreviatedCampuses.register self
end

class UMNTCRO
  def name
    "University of Minnesota Rochester"
  end
  def mascot
    "Raptor"
  end
  def abbreviation
    "UMNTCRO"
  end

  AbbreviatedCampuses.register self
end

class CollegeInTheSchools
  def name
    "College in the Schools"
  end
  def mascot
    ""
  end
  def abbreviation
    "CITS"
  end

  AbbreviatedCampuses.register self
end

Some sample test code:

puts CampusDetails.new.campus_name("CITS")
puts CampusDetails.new.campus_mascot("UMNTCRO")
puts CampusDetails.new.campus_mascot("NOSUCH")

I think the two CampusDetails methods would be better off as class methods, defined like def self.campus_name, since they don’t have any instance variables. But I kept them instance methods in this solution for easy comparison.

Contributor

roryokane commented Feb 5, 2016

Here is another solution – creating a class AbbreviatedCampuses that all campuses register themselves with using AbbreviatedCampuses.register self. All campuses also implement an abbreviation method as the single place their abbreviation is written.

With this method, when you add a new campus, you only need to change one place in the code. But you don’t have to do the slow and hard-to-understand metaprogramming with ObjectSpace or Object.const_get – you explicitly link the new campus to the new registry. And the registration line in each campus class is shorter than the three-line self.campus_like? definition with the ObjectSpace solution.

class UnknownCampus
  def name
    "Unknown Campus"
  end

  def mascot
    "Unknown Mascot"
  end
end

class AbbreviatedCampuses
  @@campus_like_classes = Hash.new(UnknownCampus)

  def self.register(klass)
    abbreviation = klass.new.abbreviation
    @@campus_like_classes[abbreviation] = klass
  end

  def self.build_from(abbreviation)
    @@campus_like_classes[abbreviation].new
  end
end

# The above two classes must be loaded first.
# The rest can load in any order.

class CampusDetails
  def campus_name(abbreviation)
    AbbreviatedCampuses.build_from(abbreviation).name
  end

  def campus_mascot(abbreviation)
    AbbreviatedCampuses.build_from(abbreviation).mascot
  end
end

class UMNTC
  def name
    "University of Minnesota Twin Cities"
  end
  def mascot
    "Gopher"
  end
  def abbreviation
    "UMNTC"
  end

  AbbreviatedCampuses.register self
end

class UMNMO
  def name
    "University of Minnesota Morris"
  end
  def mascot
    "Cougar"
  end
  def abbreviation
    "UMNMO"
  end

  AbbreviatedCampuses.register self
end

class UMNCR
  def name
    "University of Minnesota Crookston"
  end
  def mascot
    "Golden Eagle"
  end
  def abbreviation
    "UMNCR"
  end

  AbbreviatedCampuses.register self
end

class UMNTCRO
  def name
    "University of Minnesota Rochester"
  end
  def mascot
    "Raptor"
  end
  def abbreviation
    "UMNTCRO"
  end

  AbbreviatedCampuses.register self
end

class CollegeInTheSchools
  def name
    "College in the Schools"
  end
  def mascot
    ""
  end
  def abbreviation
    "CITS"
  end

  AbbreviatedCampuses.register self
end

Some sample test code:

puts CampusDetails.new.campus_name("CITS")
puts CampusDetails.new.campus_mascot("UMNTCRO")
puts CampusDetails.new.campus_mascot("NOSUCH")

I think the two CampusDetails methods would be better off as class methods, defined like def self.campus_name, since they don’t have any instance variables. But I kept them instance methods in this solution for easy comparison.

@roryokane

This comment has been minimized.

Show comment
Hide comment
@roryokane

roryokane Feb 5, 2016

Contributor

Also, if all you need from your campuses is to get their data, you could just use a Hash instead.

Though this solution doesn’t support custom class behavior, it’s a lot simpler and more concise. This solution could apply before the step in your article when you add the CollegeInTheSchools class, which is presumed to have additional behavior.

class CampusDetails
  CAMPUS_LIKES_BY_ABBR = {
    "UMNTC" => {
      name: "University of Minnesota Twin Cities",
      mascot: "Gopher",
    },
    "UMNMO" => {
      name: "University of Minnesota Morris",
      mascot: "Cougar",
    },
    "UMNCR" => {
      name: "University of Minnesota Crookston",
      mascot: "Golden Eagle",
    },
    "UMNTCRO" => {
      name: "University of Minnesota Rochester",
      mascot: "Raptor",
    },
    "CITS" => {
      name: "College in the Schools",
      mascot: "",
    },
  }
  CAMPUS_LIKES_BY_ABBR.default = {
    name: "Unknown Campus",
    mascot: "Unknown Mascot",
  }

  def campus_name(abbreviation)
    CAMPUS_LIKES_BY_ABBR[abbreviation][:name]
  end

  def campus_mascot(abbreviation)
    CAMPUS_LIKES_BY_ABBR[abbreviation][:mascot]
  end
end
Contributor

roryokane commented Feb 5, 2016

Also, if all you need from your campuses is to get their data, you could just use a Hash instead.

Though this solution doesn’t support custom class behavior, it’s a lot simpler and more concise. This solution could apply before the step in your article when you add the CollegeInTheSchools class, which is presumed to have additional behavior.

class CampusDetails
  CAMPUS_LIKES_BY_ABBR = {
    "UMNTC" => {
      name: "University of Minnesota Twin Cities",
      mascot: "Gopher",
    },
    "UMNMO" => {
      name: "University of Minnesota Morris",
      mascot: "Cougar",
    },
    "UMNCR" => {
      name: "University of Minnesota Crookston",
      mascot: "Golden Eagle",
    },
    "UMNTCRO" => {
      name: "University of Minnesota Rochester",
      mascot: "Raptor",
    },
    "CITS" => {
      name: "College in the Schools",
      mascot: "",
    },
  }
  CAMPUS_LIKES_BY_ABBR.default = {
    name: "Unknown Campus",
    mascot: "Unknown Mascot",
  }

  def campus_name(abbreviation)
    CAMPUS_LIKES_BY_ABBR[abbreviation][:name]
  end

  def campus_mascot(abbreviation)
    CAMPUS_LIKES_BY_ABBR[abbreviation][:mascot]
  end
end

IanWhitney added some commits Feb 5, 2016

Merge pull request #18 from roryokane/patch-3
Fix `handles?` in factory post by changing = to ==
Merge pull request #17 from roryokane/patch-2
Change `def…def` to `def…end` in factories post
Merge pull request #16 from roryokane/patch-1
Remove trailing whitespace in factories post
@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Feb 5, 2016

Owner

@roryokane The hash is a fine approach and probably one I'd use in some cases. I tend towards using objects over data structures in most cases, and since the post was about factories the hash approach didn't really fit.

The AbbreviatedCampuses.register self option is not one I'd ever thought of. Thank you for pointing it out. My brain doesn't think of doing method calls like that inside of class definitions.

Owner

IanWhitney commented Feb 5, 2016

@roryokane The hash is a fine approach and probably one I'd use in some cases. I tend towards using objects over data structures in most cases, and since the post was about factories the hash approach didn't really fit.

The AbbreviatedCampuses.register self option is not one I'd ever thought of. Thank you for pointing it out. My brain doesn't think of doing method calls like that inside of class definitions.

@IanWhitney

This comment has been minimized.

Show comment
Hide comment
@IanWhitney

IanWhitney Feb 5, 2016

Owner

Also, thanks for the PRs, @roryokane. They have been pushed to the site. The nice thing about writing examples in Rust is that I have tooling that will tell me if the code is correct. With Ruby I'm usually just freehanding it and occasionally checking syntax in IRB. So I make some dumb mistakes, as you noticed.

Owner

IanWhitney commented Feb 5, 2016

Also, thanks for the PRs, @roryokane. They have been pushed to the site. The nice thing about writing examples in Rust is that I have tooling that will tell me if the code is correct. With Ruby I'm usually just freehanding it and occasionally checking syntax in IRB. So I make some dumb mistakes, as you noticed.

@IanWhitney IanWhitney merged commit 234ac07 into master Apr 1, 2016

@IanWhitney IanWhitney deleted the factories_simple_to_ridiculous branch Apr 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment