if !birthdate.blank? && !location.blank? && !joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} in #{location}. #{sex} passed away on #{death.strftime("%B %e, %Y")} at the age of #{calculate_age(birthdate, death)}. #{sex} was a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{distance_of_time_in_words(joined,death)}.</p>"
elsif !birthdate.blank? && !location.blank? && !joined.blank? && death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} in #{location} and is #{time_ago_in_words(birthdate)} old. #{sex} has been a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{time_ago_in_words(joined)}.</p>"
elsif birthdate.blank? && !location.blank? && !joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} passed away on #{death.strftime("%B %e, %Y")}. #{sex} was a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{distance_of_time_in_words(joined,death)}.</p>"
elsif birthdate.blank? && !location.blank? && !joined.blank? && death.blank?
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} has been a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{time_ago_in_words(joined)}.</p>"
elsif birthdate.blank? && location.blank? && !joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{distance_of_time_in_words(joined,death)}. #{sex} passed away on #{death.strftime("%B %e, %Y")}.</p>"
elsif birthdate.blank? && location.blank? && !joined.blank? && death.blank?
"<p class='birthinfo'>#{name} has been a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{time_ago_in_words(joined)}.</p>"
elsif !birthdate.blank? && location.blank? && !joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")}. #{sex} passed away on #{death.strftime("%B %e, %Y")} at the age of #{calculate_age(birthdate, death)}. #{sex} was a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{distance_of_time_in_words(joined,death)}.</p>"
elsif !birthdate.blank? && location.blank? && !joined.blank? && death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} and is #{time_ago_in_words(birthdate)} old. #{sex} has been a member of #{link_to user.login, profile_path(user.permalink)}'s family for #{time_ago_in_words(joined)}.</p>"
elsif !birthdate.blank? && !location.blank? && joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} in #{location}. #{sex} passed away on #{death.strftime("%B %e, %Y")} at the age of #{calculate_age(birthdate, death)}. #{sex} was a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
elsif !birthdate.blank? && !location.blank? && joined.blank? && death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} in #{location} and is #{time_ago_in_words(birthdate)} old. #{sex} is a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
elsif !birthdate.blank? && location.blank? && joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")}. #{sex} passed away on #{death.strftime("%B %e, %Y")} at the age of #{calculate_age(birthdate, death)}. #{sex} was a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
elsif !birthdate.blank? && location.blank? && joined.blank? && death.blank?
"<p class='birthinfo'>#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} and is #{time_ago_in_words(birthdate)} old. #{sex} is a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
elsif birthdate.blank? && !location.blank? && joined.blank? && !death.blank?
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} passed away on #{death.strftime("%B %e, %Y")}. #{sex} was a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
elsif birthdate.blank? && !location.blank? && joined.blank? && death.blank?
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} is a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
else
"<p class='birthinfo'>#{name} is a member of #{link_to user.login, profile_path(user.permalink)}'s family.</p>"
end
Refactorings
No refactoring yet !
Mortice
July 30, 2009, July 30, 2009 08:39, permalink
No rating.
Login to rate!
Not tested fully, but you get the idea...
class Person
def initialize(name, birth_date, birthplace, joined_date, death_date, sex, user)
@name = name
@birth_date = birth_date
@birthplace = birthplace
@joined_date = joined_date
@death_date = death_date
@user = user
end
def to_s
"#{@name} #{birth_info}#{age_info}. #{sex}#{death_info} #{is_or_was} #{family_info}."
end
def birth_info
"was born #{birth_date_info} #{birthplace_info}"
end
def birth_date_info
"on #{@birth_date.strftime("%A, %B %e, %Y")}"
end
def birthplace_info
"in #{@birthplace}"
end
def age_info
" and is #{time_ago_in_words(@birth_date)} old." unless self.dead?
end
def death_info
" passed away on #{@death_date.strftime("%B %e, %Y")} at the age of #{self.age_at_death}."
end
def dead?
!death_date.blank?
end
def age_at_death
@death_date.year - @birth_date.year #TODO make more accurate
end
def is_was_or_has_been
if dead?
"was"
elsif @joined_date.blank?
"is"
else
"has been"
end
end
def family_info
"a member of #{link_to @user.login, profile_path(@user.permalink)}'s family#{since_info}"
end
def since_info
" for #{time_ago_in_words(@joined_date)}"
end
end
person = Person.new(name, birthdate, location, joined, death, sex, user)
"<p class='birthinfo'>#{person}</p>"
above fails
July 30, 2009, July 30, 2009 13:29, permalink
No rating.
Login to rate!
above refactor fails when birthdate = nil || lots of other things
above fails
July 30, 2009, July 30, 2009 13:29, permalink
No rating.
Login to rate!
above refactor fails when birthdate = nil || lots of other things
Tj Holowaychuk
July 30, 2009, July 30, 2009 14:09, permalink
No rating.
Login to rate!
This is just pure bad. Tons of duplication, and constructors should not have so many parameters, have an optional hash of options like below to make things more clear:
persion = Persion.new 'tj', :birthday => 'foo', :location => 'Victoria', ..
Muke Tever
July 30, 2009, July 30, 2009 15:23, permalink
No rating.
Login to rate!
I tried to take it apart a bit.
(Haven't tested this, but I think the idea is there.)
# first sentence
birthinfo = "<p class='birthinfo'>#{name} "
if birthdate.present? || location.present? {
birthinfo << "was born " +
"#{" on" + birthdate.strftime("%A, %B %e, %Y") unless birthdate.blank}" +
"#{" in" + location unless location.blank}" +
"#{" and is" + time_ago_in_words(birthdate) + "old" if birthdate.present? && death.blank?}" +
". "
} else {
birthinfo << member_of_info
}
# second sentence
if death.present? {
birthinfo << "#{sex} passed away on #{death.strftime("%B %e, %Y")}" +
"#{" at the age of " + calculate_age(birthdate, death) if birthdate.present?}" +
". "
}
# third sentence
if birthdate.present? || location.present? {
birthinfo << "#{sex} " + member_of_info
}
# rather than having it out there twice:
def member_of_info
returning "" do |str|
str <<
case
when joined.blank? && death.blank? then "is "
when joined.present? && death.blank? then "has been "
when death.present? then "was "
end +
"a member of #{link_to user.login, profile_path(user.permalink)}'s family"
if joined.present? {
str <<
(death.present? ?
" for #{distance_of_time_in_words(joined,death)}" :
" for #{time_ago_in_words(joined)}")
}
str << ".</p>"
end
end
Muke Tever
July 30, 2009, July 30, 2009 15:25, permalink
No rating.
Login to rate!
I seem to have thrown a stray 'else = ""' in. The heck? -.-
Sam
August 1, 2009, August 01, 2009 17:05, permalink
No rating.
Login to rate!
And this, folks, is why people really need to learn programming from assembly language on up. Coding in higher-level languages like Ruby is great from a pure productivity point of view, but it completely hides the efficacy of techniques such as the humble "table look up".
Behold . . .
# normally, I like long, expressive variable names. However, considering
# the table's volume of duplication, pragmatic concerns overrule code aesthetics.
b = birthdate.strftime("%A, %B %e, %Y") if !birthdate.blank?
d = death.strftime("%B %e, %Y") if !death.blank?
a = calculate_age(birthdate, death)
l = link_to user.login, profile_path(user.permalink)
di = distance_of_time_in_words(joined, death)
ti = time_ago_in_words(birthdate)
tj = time_ago_in_words(joined)
messages = [
"<p class='birthinfo'>#{name} was born on #{b} in #{location}. #{sex} passed away on #{d} at the age of #{a}. #{sex} was a member of #{l}'s family for #{di}.</p>",
"<p class='birthinfo'>#{name} was born on #{b} in #{location} and is #{ti} old. #{sex} has been a member of #{l}'s family for #{tj}.</p>",
"<p class='birthinfo'>#{name} was born on #{b} in #{location}. #{sex} passed away on #{d} at the age of #{a}. #{sex} was a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} was born on #{b} in #{location} and is #{ti} old. #{sex} is a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} was born on #{b}. #{sex} passed away on #{d} at the age of #{a}. #{sex} was a member of #{l}'s family for #{di}.</p>",
"<p class='birthinfo'>#{name} was born on #{b} and is #{ti} old. #{sex} has been a member of #{l}'s family for #{tj}.</p>",
"<p class='birthinfo'>#{name} was born on #{b}. #{sex} passed away on #{d} at the age of #{a}. #{sex} was a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} was born on #{b} and is #{ti} old. #{sex} is a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} passed away on #{d}. #{sex} was a member of #{l}'s family for #{di}.</p>",
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} has been a member of #{l}'s family for #{tj}.</p>",
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} passed away on #{d}. #{sex} was a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} was born in #{location}. #{sex} is a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} was a member of #{l}'s family for #{di}. #{sex} passed away on #{d}.</p>",
"<p class='birthinfo'>#{name} has been a member of #{l}'s family for #{tj}.</p>",
"<p class='birthinfo'>#{name} is a member of #{l}'s family.</p>",
"<p class='birthinfo'>#{name} is a member of #{l}'s family.</p>"
]
birthdate_bit = if birthdate.blank? then 0 else 8 end
location_bit = if location.blank? then 0 else 4 end
joined_bit = if joined.blank? then 0 else 2 end
death_bit = if death.blank? then 0 else 1 end
messages[birthdate_bit + location_bit + joined_bit + death_bit]
Sam
August 1, 2009, August 01, 2009 17:07, permalink
No rating.
Login to rate!
BTW, it's good to know that I'm so effective at coding, I can submit a refactoring before I even think of the solution. ;) From the comment's heading...
Sam
Today, -4 minutes ago, permalink
Muke Tever
September 6, 2009, September 06, 2009 03:45, permalink
No rating.
Login to rate!
I discovered a new hammer today (so every problem looks like a nail...), and even though this is an old post it felt like a nail worth pounding.
birthinfo = "<p class='birthinfo'>%s %s %s %s</p>" % [name,
if birthdate.present? || location.present? then "was born %s%s%s." % [
("on %s" % birthdate.strftime("%A, %B %e, %Y") unless birthdate.blank?),
("in %s" % location unless location.blank?),
("and is %s old" % time_ago_in_words(birthdate) if birthdate.present? && death.blank?)
] else member_of_info end,
if death.present? then "%s passed away on %s%s." % [sex, death.strftime("%B %e, %Y"),
("at the age of %d" % calculate_age(birthdate, death) if birthdate.present?)
] end,
if birthdate.present? || location.present? then "%s %s" % [sex, member_of_info] end
]
def member_of_info
"%s a member of %s's family%s." % [
death.blank? ? if joined.blank? then "is" else "has been" end : "was",
link_to(user.login, profile_path(user.permalink)),
(" for %s" % death.present? ? distance_of_time_in_words(joined,death) : time_ago_in_words(joined) if joined.present?)
]
end
The following if/elsif statement is clearly a behemoth. The purpose of it is to change the phrasing of some text based on if certain data has been filled in by the user. I feel like there's got to be a better way to do this without taking up 30+ lines of code, but I'm just not sure how since I'm trying to customize the text pretty significantly based on the data available.