Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

How can I make this massive Ruby if/elsif statement more compact and cleaner?

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.

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
like image 770
Shpigford Avatar asked Dec 09 '22 20:12

Shpigford


2 Answers

I think that you want to make all of these conditions more readable, and eliminate the repetition that exists both in your logic checks and in your string creation.

The first thing that I notice is that you repeat this all over the place:

<p class='birthinfo'>#{name} was born in

I would try to factor these out into either functions that take arguments and return formatted text, or classes that evaluate into expressions.

You're also not taking advantage of nesting at all. Instead of having every condition check the value of four expressions, you should try something like this:

if birthdate.blank?
    # half of your expressions
else
    # other half of your expressions

It might not make your code more readable, but it's worth trying out. The last thing that I'd suggest is that you might be able to cleverly rearrange your text so that it is both easy to construct piecewise and still reads well to the end-user. Here's an example:

notice = "#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")} in #{location}."

One code snippet to generate this could be:

notice = "#{name} was born on #{birthdate.strftime("%A, %B %e, %Y")}"
if !location.is_blank? 
    notice += " in #{location}"
end
notice += "."

That's much easier to read that the analogous code that you have, which would check each condition separately and make a totally different string depending on the values of the boolean variables. The worst part about the logic that you have now is that if you add a fifth variable, you have to add a lot more special cases to your code. Building up your code into independent pieces would be much easier to read and maintain.

like image 61
James Thompson Avatar answered May 24 '23 19:05

James Thompson


I'd break it down into parts. DRY. Only generate a given segment of text once. Use a StringIO to keep the string generation separable.

sio = StringIO.new("")
know_birthdate, know_location, did_join, has_died = [ birthdate, location, joined, death ].map { |s| !s.blank? }

print_death = lambda do 
  sio.print ". #{sex} passed away on #{death.strftime("%B %e, %Y")}"
end
show_birth = know_birthdate or know_location

sio.print "<p class='birthinfo'>#{name} "
if show_birth
  sio.print "was born" 
  sio.print " on #{birthdate.strftime("%A, %B %e, %Y")}" if know_birthdate
  sio.print " in #{location}" if know_location
  if has_died
    print_death[]
    sio.print " at the age of #{calculate_age(birthdate, death)}" if know_birthdate
  elsif know_birthdate 
    sio.print " and is #{time_ago_in_words(birthdate)} old"
  end
  sio.print ". #{sex} "
end
sio.print "#{(has_died ? "was" : did_join ? "has been" : "is")} a member of #{link_to user.login, profile_path(user.permalink)}'s family"
sio.print " for #{distance_of_time_in_words(joined,death)}" if did_join and has_died
print_death[] if has_died and not show_birth
sio.print ".</p>"

sio.to_s

This makes the logic much easier to follow, and makes it much easier to make changes.

like image 27
rampion Avatar answered May 24 '23 17:05

rampion