Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have three methods that are really similar but do slightly different things. This doesn't feel very DRY to me, but I can't think of a way to reduce the duplication. Here are the methods, I think they're fairly self explanatory:

# Present a date as a string, optionally without a year
# Example output: "December 1, 2011"
def date_string(year=true)
  if date
    str = "%B %e"
    str += ", %Y" if year
    date.strftime(str).gsub('  ',' ')
  else
    ""
  end
end

# Present a time as a string, optionally with a meridian (am/pm)
# Example output: "1:30 PM"
def start_time_string(meridian=true)
  if start_time
    str = "%l:%M"
    str += " %p" if meridian
    start_time.strftime(str).lstrip
  else
    ""
  end
end

# Present a time as a string, optionally with a meridian (am/pm)
# Example Output: "2:00"
def end_time_string(meridian=true)
  if end_time
    str = "%l:%M"
    str += " %p" if meridian
    end_time.strftime(str).lstrip
  else
    ""
  end
end

Each method just presents a time as a string with certain options, and if the time object they're trying to present is nil, they return an empty string.

Any ideas how to DRY this up?

share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

To complement @mjgpy3's answer, some notes:

  • def date_string(year=true). That's idiomatic Python, but in Ruby the idiomatic is def date_string(year = true). Personally I don't like positional optional values, specially booleans; when you call them you have no idea what the argument stands for. You can use an options hash instead. Do you remember the old Rails obj.save(false)? wisely changed in Rails 3 to obj.save(:validate => false).

  • How to build string with conditional values: This is the pattern I use:

    str = ["%B %e", *(", %Y" if year)].join
    
  • the else returns an empty string, why? it's better if it returns nil.

  • Check String#squish.

To sum it up, I'd write the first method like this:

def date_string(options = {})
  format = ["%B %e", *(", %Y" if options[:year])].join
  date.strftime(format).squish
end

Just as a side note: does it make sense to have all these methods in a model? (or a presenter, it doesn't matter). Think of an orthogonal approach, create helper methods available for everbody to use (and send them date object), this way you achieve real modularity.

share|improve this answer
    
The reason for returning an empty string is these methods get chained together in some places, so if one of them returns nil it actually needs to be an empty string so I can join it to another string without checking for nil. Great suggestions though, this is really helpful to consider. I think you're right about extracting it to be more general purpose formatting helpers, I'll move the code that direction. Thanks! –  Andrew Dec 20 '12 at 19:00
    
Also, string.squish is awesome, wow. Thanks for showing me that! –  Andrew Dec 20 '12 at 19:02
    
Regarding the empty string, yeah, sometimes is handy to return an empty string. But usually empty strings are not that useful, you want to let the caller decide how to handle it: method_that_may_return_nil || default_value. With empty strings you still can do it, but it's more verbose: method_that_may_return_empty_string.presence || default_value –  tokland Dec 20 '12 at 20:45
add comment

Well, I don't see much you can do with the first method (being as it has a different responsibility than the other two). You can just combine the latter two methods into one and make the time object a parameter, like such:

def time_string(time_obj, meridian=true)
  if time_obj
    str = "%l:%M"
    str += " %p" if meridian
    time_obj.strftime(str).lstrip
  else
    ""
  end
end

This should reduce complexity some, since both latter methods have essentially the same responsibility.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.