On Apr 24, 2017, at 1:56 AM, Leam Hall <leamhall@gmail.com> wrote:
On 04/23/17 23:20, Patrick Bayford wrote:
Not sure about others here, but I would find it useful to see at least a
rough idea of how your classes look. Personally, I like the superclass
idea, but know from personal experience, that designing the superclass
is difficult, mainly in respect of how much detail it should include.
This is especially important when dealing with serialized, stored data,
as changing the class/es tends to break the serializer!
Hey Patrick, thanks!
The Character class:
https://github.com/LeamHall/CT_Character_Generator/blob/master/lib/Tools/Character.rb
Uses Module CharacterTools to modify most of the data:
https://github.com/LeamHall/CT_Character_Generator/blob/master/lib/Tools/CharacterTools.rb
The Career class has the methods for running a career:
https://github.com/LeamHall/CT_Character_Generator/blob/master/lib/Tools/Career.rb
And uses data from subclasses, like:
https://github.com/LeamHall/CT_Character_Generator/blob/master/lib/Careers/Citizen.rb
Just looking at one thing and iterating on it:
# 0) Delete all trailing whitespace. Use your editor to help you code like a pro. Use paragraphs/blank-lines to separate concepts:
def self.title(character)
nobility = Hash.new
nobility["B"] = { "f" => "Dame", "m" => "Knight" }
nobility["C"] = { "f" => "Baroness", "m" => "Baron" }
nobility["D"] = { "f" => "Marquesa", "m" => "Marquis" }
nobility["E"] = { "f" => "Countess", "m" => "Count" }
nobility["F"] = { "f" => "Duchess", "m" => "Duke" }
soc = character.upp[5,1].upcase
if nobility.has_key?(soc)
if character.gender.downcase == "female"
title = nobility[soc]["f"]
character.title = title
else
title = nobility[soc]["m"]
character.title = title
end
end
return title
end
# 1) upcase & downcase is showing that you’re not normalizing data
# when it comes in / gets stored. I haven’t looked at your code,
# but chances are all this data is internal and doesn’t need ANY
# up/down case.
def self.title(character)
nobility = Hash.new
nobility["B"] = { "f" => "Dame", "m" => "Knight" }
nobility["C"] = { "f" => "Baroness", "m" => "Baron" }
nobility["D"] = { "f" => "Marquesa", "m" => "Marquis" }
nobility["E"] = { "f" => "Countess", "m" => "Count" }
nobility["F"] = { "f" => "Duchess", "m" => "Duke" }
soc = character.upp[5,1]
if nobility.has_key?(soc)
if character.gender == "female"
title = nobility[soc]["f"]
character.title = title
else
title = nobility[soc]["m"]
character.title = title
end
end
return title
end
# 2) nobility doesn't change. Pull it out and make it a constant:
NOBILITY = {
"B" => { "f" => "Dame", "m" => "Knight" },
"C" => { "f" => "Baroness", "m" => "Baron" },
"D" => { "f" => "Marquesa", "m" => "Marquis" },
"E" => { "f" => "Countess", "m" => "Count" },
"F" => { "f" => "Duchess", "m" => "Duke" },
}
def self.title(character)
soc = character.upp[5,1]
if NOBILITY.has_key?(soc)
if character.gender == "female"
title = NOBILITY[soc]["f"]
character.title = title
else
title = NOBILITY[soc]["m"]
character.title = title
end
end
return title
end
# 3) You're using data to make a decision w/ soc, but then skip that
# concept for gender. And then duplicate code entirely:
NOBILITY = {
"B" => { "f" => "Dame", "m" => "Knight" },
"C" => { "f" => "Baroness", "m" => "Baron" },
"D" => { "f" => "Marquesa", "m" => "Marquis" },
"E" => { "f" => "Countess", "m" => "Count" },
"F" => { "f" => "Duchess", "m" => "Duke" },
}
def self.title(character)
soc = character.upp[5,1]
gender = character.gender[0]
if NOBILITY.has_key?(soc)
title = NOBILITY[soc][gender]
character.title = title
end
return title
end
# 4) Don't use return when it isn't an early exit
NOBILITY = {
"B" => { "f" => "Dame", "m" => "Knight" },
"C" => { "f" => "Baroness", "m" => "Baron" },
"D" => { "f" => "Marquesa", "m" => "Marquis" },
"E" => { "f" => "Countess", "m" => "Count" },
"F" => { "f" => "Duchess", "m" => "Duke" },
}
def self.title(character)
soc = character.upp[5,1]
gender = character.gender[0]
if NOBILITY.has_key?(soc)
title = NOBILITY[soc][gender]
character.title = title
end
title
end
# 5) Everything can be collapsed to the one real concept here:
NOBILITY = {
"B" => { "f" => "Dame", "m" => "Knight" },
"C" => { "f" => "Baroness", "m" => "Baron" },
"D" => { "f" => "Marquesa", "m" => "Marquis" },
"E" => { "f" => "Countess", "m" => "Count" },
"F" => { "f" => "Duchess", "m" => "Duke" },
}
def self.title(character)
soc = character.upp[5,1]
gender = character.gender[0]
character.title = NOBILITY[soc][gender] if NOBILITY.has_key?(soc)
end
# 6) Which shows one real problem, this is called `title` but really
# sets it and returns it, every time. Either rename it or just
# return and set it somewhere else.
NOBILITY = {
"B" => { "f" => "Dame", "m" => "Knight" },
"C" => { "f" => "Baroness", "m" => "Baron" },
"D" => { "f" => "Marquesa", "m" => "Marquis" },
"E" => { "f" => "Countess", "m" => "Count" },
"F" => { "f" => "Duchess", "m" => "Duke" },
}
def self.calculate_title(character)
soc = character.upp[5,1]
gender = character.gender[0]
character.title = NOBILITY[soc][gender] if NOBILITY.has_key?(soc)
end
# 7) The other real problem is that this is a class method on a module
# that is acting on Character. This isn't OO. This is procedural.
# It is the responsibility of a Character to know its title and
# shouldn't be externalized.
Unsubscribe: <mailto:ruby-talk-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-talk>
