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

Question

The following if the statement / elsif is clearly a hippo. The purpose of this is to change the wording of some text based on whether certain data has been filled in by the user. I feel that there should be a better way to do this without taking up more than 30 lines of code, but I'm just not sure how since I try to tweak the text quite substantially based on the available data.

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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} 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)} family.</p>" else "<p class='birthinfo'>#{name} is a member of #{link_to user.login, profile_path(user.permalink)} family.</p>" end 

Answer 1

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

The first thing I notice is that you repeat it everywhere:

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

I would try to decompose them into any functions that take arguments and return formatted text, or classes that are evaluated in expressions.

You also do not use the jack at all. Instead of having each condition check the meaning of four expressions, you should try something like this:

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

This may not make your code more readable, but worth a try. The last thing I would like to offer is that you could skillfully rebuild your text so that it was easily built piecewise and is still well read by end users. Here is an example:

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

One piece of code to create it could be:

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

It is much easier to read that you have similar code that will check each condition separately and make a completely different line depending on the values ​​of Boolean variables. The worst part is that you have a logic in that if you add a fifth variable, you will have to add a lot more special cases to your code. Creating code in independent parts would be much easier to read and maintain.

Answer 2

I would break it into pieces. DRY . Create only one segment of text at a time. Use StringIO to save the generation of strings.

 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)} 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 greatly simplifies the logic and greatly facilitates making changes.

Answer 3

You can put the strings in an array, then create an array index based on the fact that each of your variables is empty:

 strings = [ "<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)} family for #{distance_of_time_in_words(joined,death)}.</p>", "<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)} family for #{time_ago_in_words(joined)}.</p>", "<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)} family.</p>", "<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)} family.</p>", "<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)} family for #{distance_of_time_in_words(joined,death)}.</p>", "<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)} family for #{time_ago_in_words(joined)}.</p>", "<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)} family.</p>", "<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)} family.</p>", "<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)} family for #{distance_of_time_in_words(joined,death)}.</p>", "<p class='birthinfo'>#{name} was born in #{location}. #{sex} has been a member of #{link_to user.login, profile_path(user.permalink)} family for #{time_ago_in_words(joined)}.</p>", "<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)} family.</p>", "<p class='birthinfo'>#{name} was born in #{location}. #{sex} is a member of #{link_to user.login, profile_path(user.permalink)} family.</p>", "<p class='birthinfo'>#{name} was a member of #{link_to user.login, profile_path(user.permalink)} family for #{distance_of_time_in_words(joined,death)}. #{sex} passed away on #{death.strftime("%B %e, %Y")}.</p>", "<p class='birthinfo'>#{name} has been a member of #{link_to user.login, profile_path(user.permalink)} family for #{time_ago_in_words(joined)}.</p>", "", # This case was missing from your code. (Where all are blank except 'death'.) "<p class='birthinfo'>#{name} is a member of #{link_to user.login, profile_path(user.permalink)} family.</p>" ] index = 0 index += 1 unless death.blank? index += 2 unless joined.blank? index += 4 unless location.blank? index += 8 unless birthdate.blank? return strings[index] 

Answer 4

Here's how I do it:

 ret = [] ret << "<p class='birthinfo'>#{name}" ret << "was born on #{birthdate.strftime("%A, %B %e, %Y")}" unless birthdate.blank? ret << "in #{location}." unless location.blank? ret << sex ret << "passed away on #{death.strftime("%B %e, %Y")}" unless death.blank? ret << "at the age of #{calculate_age(birthdate, death)}" ret << "#{sex} was a member of #{link_to user.login, profile_path(user.permalink)} family" ret << 'for #{distance_of_time_in_words(joined,death)}" unless joined.blank? || death.blank? ret << '.</p>' ret.join(' ') 

Answer 5

You can use deMorgan's theorem to rewrite a query so that it is evaluated more efficiently.
You can also decompose common subexpressions into external if statements. However, when you do this, you are likely to sacrifice clarity of intent.
At present, it is probably better to write a clear, if long statement, rather than a short, incomprehensible one.

Answer 6

I would have the desire to write convenient methods that describe each of these states, if there is a simple descriptive term for them. This will not reduce the amount of code, but it will result in more readable code. This code is almost impossible to read.

If you look at this, I'm not sure what descriptive names for the usability methods you could come up with. You just need to deal with ugliness, given what you are trying to do.

Answer 7

One thing I can think of is not to execute each query, execute all queries and assign the results of Boolean variables, and then check the boolean values. This does not reduce the LOC, but reduces the line length. Forget what I said earlier about switch statements, which was not at all useful. What you can try to do is break each condition into several methods. The first thing that is checked in this case, if, will still be present, but it will call one of two methods for the following condition and so on and so on. This will actually expand LOC (which is not so important), but readability will be increased. This is not an optimal solution, but it will work.