Trying to learn the Factory pattern. What can be improved on the
"Crew" class as a Factory?
···
###
class Crew
def initialize(crew_number) @crew = []
crew_number.times do
ncp = Character.new
career = ["Merchant", "Navy"].sample
terms = rand(1..3)
new_crewperson(career, terms, ncp) @crew << ncp
end
end
def show_crew
counter = 1 @crew.each do |c|
print("Crewperson #%d is %d years old. Former %s, UPP: %s.\n" %
[counter, c.age, c.career, c.upp])
counter += 1
end
end
def new_crewperson(career,terms,ncp)
if career == "Merchant"
Merchant.new(ncp, terms)
elsif career == "Navy"
Navy.new(ncp, terms)
end
end
end
class Character
attr_accessor :age, :career, :name, :upp
def initialize @age = 14
end
end
class Career
def initialize(char, terms = 1) @char = char @terms = terms
run_career
end
def run_career @char.age += @terms * 4
end
end
class Merchant < Career
def run_career
super() @char.upp = "777888" @char.career = self.class.to_s
end
end
class Navy < Career
def run_career
super() @char.upp = "777777" @char.career = self.class.to_s
end
end
### Actually do stuff
my_crew = Crew.new(6)
my_crew.show_crew
Trying to learn the Factory pattern. What can be improved on the
"Crew" class as a Factory?
First thing I notice, it is not a pure factory because it seems to
hold application state as well. The factory uses the factory method
new_crewperson while initializing itself. I'd say that is a rather
unusual pattern.
Then, if you use the Career subclass instead of a String your code
becomes more flexible (i.e. you just need to add another type and not
change class Crew) and the factory method is superfluous.
If you now also use Class#inherited to automatically collect all
subclasses of Career you can even get rid of the explicit mentioning
of the list in Crew#initialize:
1 #!/usr/bin/ruby
2
3 class Career
4 ALL =
5 end
6
7 module CareerList
8 def inherited(cl)
9 ::Career::ALL << cl
10 cl.extend CareerList
11 end
12 end
13
14 class Career
15 extend CareerList
16 end
17
18 class Foo < Career
19 end
20
21 class Bar < Foo
22 end
23
24 p Career::ALL
Of course you can use other mechanisms to collect sub classes of
Career, i.e. to avoid ALL being publicly available and writable.
class Crew
def initialize(crew_number) @crew =
crew_number.times do
ncp = Character.new
career = ["Merchant", "Navy"].sample
terms = rand(1..3)
new_crewperson(career, terms, ncp) @crew << ncp
end
end
Here you can do
@crew = crew_number.times.map do
...
end
Also, I would pull out the character name Array creation out of the
loop if you do not apply the automatically collecting approach.
def show_crew
counter = 1 @crew.each do |c|
print("Crewperson #%d is %d years old. Former %s, UPP: %s.\n" %
[counter, c.age, c.career, c.upp])
counter += 1
end
end
#each_with_index could be used here. And why not use printf?
Kind regards
robert
···
On Sat, Feb 24, 2018 at 5:44 PM, leam hall <leamhall@gmail.com> wrote:
Trying to learn the Factory pattern. What can be improved on the
"Crew" class as a Factory?
###
class Crew
def initialize(crew_number) @crew =
crew_number.times do
I would make sure crew number is indeed a number or can be changed to one ( .to_i ). Also I am not sure I would create the crews in the initialize method. I would create them in an auxiliary function.
Good points!
@crew\.each do |c|
print\("Crewperson \#%d is %d years old\. Former %s, UPP: %s\.\\n" %
\[counter, c\.age, c\.career, c\.upp\]\)
counter \+= 1
end
I would use puts and #{var} syntax. This is not C or python2. Also, I would use .each_with_index but that's just me.
I have to retain my C and Python skills. My coding style reflects that.
class Merchant < Career
def run_career
super()
nitpick. Remove the ().
I read somewhere that super and super() were different but I can't find that reference. If not then I'll remove the ().
Hmm...good point. The Factory should make a group of related objects; crew members based on certain Careers they have had. For example, a ship needs Merchants, Navy, or Scouts. A mercenary group would need Army or Marines. The "crew_number" is determined by the size of the ship or the size of the mercenary group.
One of the things I'm still stumbling through is the ability to take a pre-existing Character and send them through a "Career". In that case the Career seems sort of a Factory as well, assuming I understand the idea. Not sure on that, though.
Leam
···
On 02/25/2018 01:22 PM, Robert Klemme wrote:
On Sat, Feb 24, 2018 at 5:44 PM, leam hall <leamhall@gmail.com> wrote:
Trying to learn the Factory pattern. What can be improved on the
"Crew" class as a Factory?
First thing I notice, it is not a pure factory because it seems to
hold application state as well. The factory uses the factory method
new_crewperson while initializing itself. I'd say that is a rather
unusual pattern.
Then, if you use the Career subclass instead of a String your code
becomes more flexible (i.e. you just need to add another type and not
change class Crew) and the factory method is superfluous.
If you now also use Class#inherited to automatically collect all
subclasses of Career you can even get rid of the explicit mentioning
of the list in Crew#initialize:
Trying to learn the Factory pattern. What can be improved on the
"Crew" class as a Factory?
First thing I notice, it is not a pure factory because it seems to
hold application state as well. The factory uses the factory method
new_crewperson while initializing itself. I'd say that is a rather
unusual pattern.
Then, if you use the Career subclass instead of a String your code
becomes more flexible (i.e. you just need to add another type and not
change class Crew) and the factory method is superfluous.
If you now also use Class#inherited to automatically collect all
subclasses of Career you can even get rid of the explicit mentioning
of the list in Crew#initialize:
Hmm...good point. The Factory should make a group of related objects; crew
members based on certain Careers they have had. For example, a ship needs
Merchants, Navy, or Scouts. A mercenary group would need Army or Marines.
The "crew_number" is determined by the size of the ship or the size of the
mercenary group.
But then the list of career options would also have to be an input
(i.e. method argument), wouldn't it? Or you would need a template per
type of ship that covers all the variable parts and can be used to
create one type of ship.
One of the things I'm still stumbling through is the ability to take a
pre-existing Character and send them through a "Career". In that case the
Career seems sort of a Factory as well, assuming I understand the idea. Not
sure on that, though.
But what does "send through a career" really mean? To me it does not
sound like a factory which is supposed to do a one off job (i.e.
create something) but rather like a manipulator that applies varying
state transitions to the character.
I'm not going to be of any help here because I've literally never used the Factory pattern. Frankly, I'm not sure it's of much use in Ruby -- although of course this certainly does not preclude your learning it.
Some of the Gang Of Four patterns don't really map to Ruby. They were developed for a very different sort of language, without (to be completely biased) the advantages of Ruby. For example, the Iterator pattern is built into the language itself...
I appreciate the feedback! Some of the comments make me think I've not communicated the overall design or intent well. I pulled ideas out of a larger program to focus on the Factory pattern.
The overall goal is to have fun and gain a deeper understanding of Object Oriented Design and Programming. While some things like Character or Dice could be better as a Hash or Method, that won't help me learn OO stuff. I'm trying to understand patterns; I know they exist and they can be over-used.
To be honest, the Factory may not be the way to go. I am reading "Design Patterns in Ruby" by Russ Olsen and trying to replicate the Python code below. Here's the idea, with links to the larger code base.
An instance of Character holds data character object.
If needed, Character::generate "fills in" missing information about a Character.
Characters are sometimes stored in a database.
A character "goes through" one or more Careers. "Goes through" modifies the character and can add fields like equipment.
(Pardon the Python; this is what I'm trying to replicate)
A group of people may need to be generated for a starship or mercenary group. The size of the ship or group determines what types of Career are best and what skills are required.
(Future State)
Characters will be stored in the database automatically.
A ship crew will be assigned to a particular ship.
A Mercenary unit will have a personnel roster.
