Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a method on my user model to calculate the user's age and return a human readable string. My user's can be between 1 month old and above so the returned string is different depending on if the person is "2 months old" or "1 year old" or "2 years and 3 months old".

I have reviewed a few SO posts to come to this solution. Is there anything I am missing? Leap years?

def age
    dob = self.date_of_birth

    # if a date of birth is not nil
    if dob != nil 

      # get current date  
      now = Date.current

      # has person had their birthday yet this year
      had_birthday = ((now.month > dob.month || (now.month == dob.month && now.day >= dob.day)) ? true : false) 

      # if yes then subtract this year from birthday year, if not then also subtract 1 to get how many full years old they are
      years = now.year - dob.year - (had_birthday ? 0 : 1)

      # get the calendar month difference from birthdya calendar month and today's calendar month.  if they have not had their birthdya yet then subtract the difference from 12
      months = had_birthday ? now.month - dob.month : 12 - (now.month - dob.month)

      # for under 1 year olds
      if years == 0
        return months > 1 ? months.to_s + " months old" : months.to_s + " month old"  

      # for 1 year olds
      elsif years == 1
        return months > 1 ? years.to_s + " year and " + months.to_s + " months old" : years.to_s + " year and " + months.to_s + " month old" 

      # for older than 1
      else
        return months > 1 ? years.to_s + " years and " + months.to_s + " months old" : years.to_s + " years and " + months.to_s + " month old"
      end

    # No date of birth saved so can not calculate age
    else
      return "No Date of Birth"
    end
  end
share|improve this question
    
I'd just use github.com/abhidsm/time_diff – tokland Mar 4 at 23:37
up vote 3 down vote accepted

Make an additional function to handle plural

In-lining the extra logic to handle the plural is messy, I suggest a small separate method:

def plural(word, value)
  "#{value.to_s} #{word}#{value > 1 ? 's' : ''}"
end

Reduce nesting by returning early

If you write

if dob == nil
  return "No Date of Birth"
end

You can reduce nesting in the rest of the function and highlight the handling of the special case.

share|improve this answer
    
Great suggestions thank you! – MicFin Mar 5 at 19:44
1  
Or return "No Date of Birth" if dob.nil? to reduce even more nesting. :) – Greg Burghardt Jun 1 at 21:44

I think this line:

months = had_birthday ? now.month - dob.month : 12 - (now.month - dob.month)

should be:

months = had_birthday ? now.month - dob.month : 12 - (dob.month - now.month)

For example, if today is 2016-June-31 and the dob is 2015-Dec-31, now.month - dob.month will be "-6". 12 - (-6) = 18 making the result "18 months old" instead of "6 months old".

share|improve this answer
    
Welcome to Code Review! I'm not sure why someone downvoted you, this is a real bug that you've found, nice work! – SuperBiasedMan Jun 1 at 10:37
    
Thanks @SuperBiasedMan! I was also wondering why it was downvoted – dhackdheolu Jun 2 at 13:11

To facilitate unit testing, I recommend putting this code in a parameterized function in a helper module. Your application can then call that function with self.date_of_birth as an argument.

There are a lot of conditionals in your logic. If you want an interval with month resolution, then it would be a lot simpler to compute the difference in months first. It would also be smarter to compose the text rather than enumerate all the possible combinations.

today would be a better variable name than now, since the Date object only has day resolution.

By English pluralization rules, "0 months old" would be more correct than "0 month old".

module Helper
  def age(dob, today=nil)
    return "No date of birth" if dob.nil?
    today ||= Date.current

    month_diff = (12 * today.year + today.month) - (12 * dob.year + dob.month)
    y, m = month_diff.divmod 12

    y_text =      (y == 0) ? nil : (y == 1) ? '1 year'  : "#{y} years"
    m_text = y && (m == 0) ? nil : (m == 1) ? '1 month' : "#{m} months"
    [y_text, m_text].compact.join(' and ') + ' old'
  end
end
share|improve this answer
1  
small improvement: y, m = month_diff.divmod 12. also, won't this make someone whose birthday was the last day of a month "1 month" old on the first of the next month? and someone born on the first will still be 0 on the last day of the month? in fairness, it's not clear from the OP what the desired behavior is – Jonah Mar 5 at 4:23
    
Great suggestion and yes I didn't identify all of the edge cases I should for testing. Thanks for identifying the first and last of month scenario. – MicFin Mar 5 at 19:46

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.