1
\$\begingroup\$

I have a class with 2 date properties: FirstDay and LastDay. LastDay is nullable. I would like to generate a string in the format of "x year(s) y day(s)". If the total years are less than 1, I would like to omit the year section. If the total days are less than 1, I would like to omit the day section. If either years or days are 0, they should say "day/year", rather than "days/years" respectively.

Examples:
2.2 years:             "2 years 73 days"
1.002738 years:   "1 year 1 day"
0.2 years:             "73 days"
2 years:                "2 years"

What I have works, but it is long:

private const decimal DaysInAYear = 365.242M;

public string LengthInYearsAndDays
{
    get
    {
        var lastDay = this.LastDay ?? DateTime.Today;
        var lengthValue = lastDay - this.FirstDay;

        var builder = new StringBuilder();

        var totalDays = (decimal)lengthValue.TotalDays;
        var totalYears = totalDays / DaysInAYear;
        var years = (int)Math.Floor(totalYears);

        totalDays -= (years * DaysInAYear);
        var days = (int)Math.Floor(totalDays);

        Func<int, string> sIfPlural = value =>
            value > 1 ? "s" : string.Empty;

        if (years > 0)
        {
            builder.AppendFormat(
                CultureInfo.InvariantCulture,
                "{0} year{1}",
                years,
                sIfPlural(years));

            if (days > 0)
            {
                builder.Append(" ");
            }
        }

        if (days > 0)
        {
            builder.AppendFormat(
                CultureInfo.InvariantCulture,
                "{0} day{1}",
                days,
                sIfPlural(days));
        }

        var length = builder.ToString();
        return length;
    }
}

Is there a more concise way of doing this (but still readable)?

\$\endgroup\$
  • 1
    \$\begingroup\$ Is there a reason why you are using a solar year (365.242 days) vs a calendar year (365 or 366 days) - not going to make a huge difference, but just looks odd since you are talking about days and calendar years but using a solar year as the denominator in the equation? \$\endgroup\$ – psubsee2003 Apr 11 '13 at 20:51
  • 1
    \$\begingroup\$ You might want to check out this answer. \$\endgroup\$ – Jeff Vanzella Apr 11 '13 at 20:58
2
\$\begingroup\$

Overall, your code doesn't look bad. You have good use of white space and indentation. I found your variable names to be a little confusing (what's the difference between totalDays and days? Without actually digging into the code, it's not obvious).

I like that you are using StringBuilder for concatenating strings, that is a good habit to get into.

I think you are doing too much manipulation on the years and days. I was able to take your 5 lines of code to calculate the years and days, and make it 3

var totalDays = Math.Floor(lengthValue.TotalDays);
var years = totalDays / DaysInAYear;        
var days = totalDays % DaysInAYear;

Years will be totalDays divided by the number of days in a year. Days will be the remainder of the same division.

I would move the pluralize out into its own method. There is no need to use an anonymous method in this instance.

You are repeating yourself when you are creating the string. If you look closely, the code is almost identical. The differences are easily passed in as variables. The way I corrected this is to create a method called CreateWords. This method returns the formatted string from variables passed in:

private string CreateWords(int value, string measure)
{
    if (value == 0) return string.Empty;

    return string.Format("{0} {1}{2}",
                value,
                measure,
                PluralSuffix(value));
}

private string PluralSuffix(int value)
{
    return value > 1 ? "s" : string.Empty
}

Your main method would then call:

builder.Append(CreateWords(years, "year"));
builder.Append(CalculateSpaceCharacter(days));
builder.Append(CreateWords(days, "day"));

where CalculateSpaceCharacter would look like:

private static string CalculateSpaceCharacter(int value)
{
    return value > 0 ? " " : string.Empty;
}

And finally, there is no need to assign the length variable at the end. Just return builder.ToString()

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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