3
\$\begingroup\$

I found another user asking for an application to be written, it sounded simple enough and a good way to "work out" my C# "muscles" and try to get them into a better "shape". I really liked the idea of learning the difference between people's ages so I decided I would code this, I did enlist the help of Stack Overflow of which I have given attribution for the code I used.

Here is the original criteria

For example:

Please enter the number of siblings: 3 
Please enter date of birth of sibling 1: 01-01-1990 
Please enter date of birth of sibling 2: 05-03-1995
Please enter date of birth of sibling 3: 08-05-1998 
Age of sibling 1 is: 25 years 2 months 19 days 
Age of sibling 2 is: 20 years 0 months 15 days 
Age of sibling 3 is: 16 years 10 months 12 days 
Difference between sibling 1 and 2 is: 05 years 2 months and 4 days 
Difference between sibling 2 and 3 is: 03 years 02 months and 3 days

This is only a console application (so far).

I started out writing the code in Main and soon realized that I wanted some custom objects, so I first created a Human Interface (nothing special in it, yet) which holds a simple DOB, in the future this describe other things and may possibly need to be named something else (most likely). The Sibling Class implements the Human Interface (and not much more, yet).

Human.cs

interface Human
{
    DateTime DOB { get; set; }
}

Sibling.cs

class Sibling : Human
{
    public DateTime DOB { get; set; }
    public Sibling (DateTime dateOfBirth)
    {
        this.DOB = dateOfBirth;
    }
}

I also made use of some DateTime manipulation, along with a struct of someone else's creation (it came in rather handy)

DateTimeExtensions.cs

static class DateTimeExtensions
{
    public static int AgeInYears (DateTime age1, DateTime age2)
    {
        return Math.Abs(age1.Year - age2.Year);
    }
    public static int AgeInMonths (DateTime age1, DateTime age2)
    {
         return ((age1.Year - age2.Year) * 12) + (age1.Month - age2.Month);
    }
    public static int AgeInDays (DateTime age1, DateTime age2)
    {
        return Convert.ToInt32(((age1.Year - age2.Year) * 365.25) + (age1.DayOfYear - age2.DayOfYear));
    }

    public static int AgePartYears(DateTime age1, DateTime age2)
    {
        return DateTimeSpan.CompareDates(age1, age2).Years;
    }

    public static int AgePartMonths(DateTime age1, DateTime age2)
    {
        return DateTimeSpan.CompareDates(age1, age2).Months;
    }

    public static int AgePartDays(DateTime age1, DateTime age2)
    {
        return DateTimeSpan.CompareDates(age1, age2).Days;
    }
}

DateTimeSpan Struct

resides in DateTimeExtensions currently

/// <summary>
/// http://stackoverflow.com/a/9216404/1214743
/// http://stackoverflow.com/users/189950/kirk-woll
/// answered Feb 9 '12 at 18:14
/// </summary>
public struct DateTimeSpan
{
    private readonly int years;
    private readonly int months;
    private readonly int days;
    private readonly int hours;
    private readonly int minutes;
    private readonly int seconds;
    private readonly int milliseconds;

    public DateTimeSpan(int years, int months, int days, int hours, int minutes, int seconds, int milliseconds)
    {
        this.years = years;
        this.months = months;
        this.days = days;
        this.hours = hours;
        this.minutes = minutes;
        this.seconds = seconds;
        this.milliseconds = milliseconds;
    }


    public int Years { get { return years; } }
    public int Months { get { return months; } }
    public int Days { get { return days; } }
    public int Hours { get { return hours; } }
    public int Minutes { get { return minutes; } }
    public int Seconds { get { return seconds; } }
    public int Milliseconds { get { return milliseconds; } }

    enum Phase { Years, Months, Days, Done }

    public static DateTimeSpan CompareDates(DateTime date1, DateTime date2)
    {
        if (date2 < date1)
        {
            var sub = date1;
            date1 = date2;
            date2 = sub;
        }

        DateTime current = date1;
        int years = 0;
        int months = 0;
        int days = 0;

        Phase phase = Phase.Years;
        DateTimeSpan span = new DateTimeSpan();

        while (phase != Phase.Done)
        {
            switch (phase)
            {
                case Phase.Years:
                    if (current.AddYears(years + 1) > date2)
                    {
                        phase = Phase.Months;
                        current = current.AddYears(years);
                    }
                    else
                    {
                        years++;
                    }
                    break;
                case Phase.Months:
                    if (current.AddMonths(months + 1) > date2)
                    {
                        phase = Phase.Days;
                        current = current.AddMonths(months);
                    }
                    else
                    {
                        months++;
                    }
                    break;
                case Phase.Days:
                    if (current.AddDays(days + 1) > date2)
                    {
                        current = current.AddDays(days);
                        var timespan = date2 - current;
                        span = new DateTimeSpan(years, months, days, timespan.Hours, timespan.Minutes, timespan.Seconds, timespan.Milliseconds);
                        phase = Phase.Done;
                    }
                    else
                    {
                        days++;
                    }
                    break;
            }
        }
        return span;
    }
}

I haven't really dove into the Struct yet to see if there was anything that I could make better, I really want to create some unit tests so that I don't have to run the code every time I want to test something in the code.

and last but not least, the ugliest part of any application the Main.

Program.cs

static void Main(string[] args)
{
    Console.WriteLine("Please enter the number of siblings");
    var siblingCount = new int();
    int.TryParse(Console.ReadLine(), out siblingCount);
    var siblings = new List<Sibling>();
    for (int i = 1; i < siblingCount + 1; i++)
    {
        Console.Write("Please Enter the date of birth for sibling " + i.ToString() + ":");
        siblings.Add(new Sibling(DateTime.Parse(Console.ReadLine())));
    }
    siblings = siblings.OrderByDescending(x => x.DOB).ToList();

    for (int i = 1; i < siblings.Count + 1; i++)
    {
        var diff = DateTimeSpan.CompareDates(siblings[i - 1].DOB, DateTime.Now);
        Console.WriteLine("Age of sibling " + i + " is: " + diff.Years + " years, " + diff.Months + " months and " + diff.Days + " days.");
    }

    for (var i = 1; i < siblingCount; i++)
    {
        var diff = DateTimeSpan.CompareDates(siblings[i - 1].DOB, siblings[i].DOB);
        Console.WriteLine("Difference between sibling " + i.ToString() + " and " + (i + 1).ToString() + " is " +
            diff.Years + " years, " + diff.Months + " months and " + diff.Days + " days.");
    }

    Console.ReadLine();
}
  • What do you think?
  • How can I clean up the Console.WriteLine()'s all over the place?
  • I know that the next step is exception handling, is my code supportive of this next step?
  • Is there anything that I could do to get my code ready for unit testing?
\$\endgroup\$
  • 1
    \$\begingroup\$ Make sure your unit tests cover leap years. \$\endgroup\$ – Thorsal Mar 7 '16 at 9:20
  • \$\begingroup\$ I don't think you need DateTimeSpan at all. Converting time differences from one format to another isn't that difficult. \$\endgroup\$ – Barry Carter Mar 8 '16 at 16:07
  • \$\begingroup\$ I look forward to the Review, @BarryCarter. \$\endgroup\$ – Malachi Mar 8 '16 at 16:51
  • \$\begingroup\$ Are you going to tweak it based on @Johan_Larsson's review, or should I review it (or more likely rewrite it) as is? \$\endgroup\$ – Barry Carter Mar 8 '16 at 16:53
  • 1
    \$\begingroup\$ Why Human.cs ? I want to calculate ages of my cats :) \$\endgroup\$ – Lost_In_Library Mar 28 '16 at 11:58
1
\$\begingroup\$

Human.cs

  • Why is there a set method? I don't expect date of birth to change. This is a big deal, state and data are very different things. Always default to making things immutable/readonly.
  • I don't like DOB, use DateOfBirth here. A good rule of thumb is that length of names should be proportional to scope. As a variable in a method dob may be fine, as a member in a public interface not so much.
  • The convention in C# is naming interfaces IHuman. IDateOfBirth may be a better name btw.
  • Good that the interface is minimal, this will allow for it to be used on things like public class Dog : IDateOfBirth

Sibling.cs

  • Given we make date of birth readonly we can add some validation (if it makes sense in your application):

    internal class Sibling : IDateOfBirth
    {
        public Sibling(DateTime dateOfBirth)
        {
            if (dateOfBirth > DateTime.UtcNow)
            {
                throw new ArgumentException("Date of birth cannot be in the future", nameof(dateOfBirth));
            }
    
            if (dateOfBirth < DateTime.UtcNow.AddYears(-150))
            {
                throw new ArgumentException("Date of birth must be less than 150 years from now", nameof(dateOfBirth));
            }
    
            this.DateOfBirth = dateOfBirth;
        }
    
        public DateTime DateOfBirth { get; }
    }
    

    • The convention in C# is to specify the internal explicitly. Not important, just mentioning it.

DateTimeSpan.cs

  • Good that you link the source in a comment.
  • With C#6 you can clean it up using get only properties. Nothing wrong with public readonly fields either, might be controversial idk.
  • Skipping the rest of the code from the SO answer.

Program.cs

  • var siblingCount = new int(); use var siblingCount = 0; as it reads clearer.
  • You may want to handle the case where the user inputs an illegal number. `int.TryParse() returns a bool so you can do:

    if (!int.TryParse(Console.ReadLine(), out siblingCount))
    {
        // handle invalid input
    }
    
  • Avoid concatenating strings with +. With C#6 you can use string interpolation like this:

    for (var i = 0; i < siblingCount; i++)
    {
        Console.Write($"Please Enter the date of birth for sibling {i + 1}:");
        var line = Console.ReadLine();
        DateTime dateOfBirth;
        if (!DateTime.TryParse(line, out dateOfBirth))
        {
            // handle invalid input
        }
    
        siblings.Add(new Sibling(dateOfBirth));
    }
    
  • DateTime has TryParse and TryParseExact which may be a better fit for this. Parse will crash your program with a FormatExceptionif user inputs an invalid date.
  • Storing var line = Console.ReadLine(); in a variable is nice for debugging.
  • Prefer to loop over the collection

    for (var i = 0; i < siblings.Count; i++)
    {
        var sibling = siblings[i];
        var diff = DateTimeSpan.CompareDates(sibling.DateOfBirth, DateTime.Now);
        Console.WriteLine($"Age of sibling {i + 1} is: {diff.Years} years, {diff.Months} months and {diff.Days} days.");
    }
    

Testing

  • The logic in DateTimeExtensions and DateTimeSpanis very easy to test.

Summary

The public set for date of birth is the biggest issue with this code.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to review this code, I will check it out in more detail when I get back to my machine. \$\endgroup\$ – Malachi Mar 6 '16 at 16:45

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.