2
\$\begingroup\$

Per the title date only class (or struct?).

.NET does not offer a date only data type.

I get there is time zone dynamic but then you have daylight-savings that can still kill you. I do document management and we just have a document with a date.

// minimal limited test
DateOnly date1 = new DateOnly(1901, 2, 1);
DateOnly date2 = new DateOnly(1902, 3, 2);
DateOnly date3 = new DateOnly(1902, 3, 2);
Debug.WriteLine(date1.GetDayDiff(date2));  // 365 + 29 = 394
Debug.WriteLine(date2.GetDayDiff(date1));
Debug.WriteLine(date2.CompareTo(date1));
Debug.WriteLine(date1.CompareTo(date2));
Debug.WriteLine(date2.CompareTo(date3));
// end test

public class DateOnly: Object, IComparable
{
    private Dictionary<int, int> MonthDay = new Dictionary<int, int>() { { 1, 31 }, { 2, 29 }, { 3, 31 }, { 4, 30 },  { 5, 31 },  { 6, 30 }
                                                                       , { 7, 31 }, { 8, 31 }, { 9, 30 }, { 10, 31 }, { 11, 30 }, { 12, 31 } };
    public override int GetHashCode() { return (Int32)Days; }
    public override bool Equals(Object obj)
    {
        if (obj == null || GetType() != obj.GetType())
            return false;

        DateOnly other = (DateOnly)obj;
        return (this.Days == other.Days);
    }
    public int CompareTo(object obj)
    {
        if (obj == null)
        {
            throw new ArgumentNullException();
        }
        if (obj is DateOnly)
        {
            DateOnly other = (DateOnly)obj;
            if (other == null)
            {
                throw new ArgumentNullException();
            }
            return (this.Days).CompareTo(other.Days);
        }
        else
        {
            throw new ArgumentNullException();
        }
    }
    public override string ToString()
    {
        return string.Format("{0}/{1}/{2}", Year, Month, Day);
    }
    public Int64 GetDayDiff(DateOnly Date)
    {
        Int64 getDayDiff = (Int64)this.Days - (Int64)Date.Days;
        return (getDayDiff);
    }
    public UInt32 Days    { get; private set; }
    public int Year       { get; private set; }
    public int Month      { get; private set; }
    public int Day        { get; private set; }
    public UInt32 MaxDays { get; private set; } 
    public DateOnly(int year, int month, int day)
    {
        if (!ValidateDate(year, month, day))
            throw new ArgumentOutOfRangeException("invalid date");
        MaxDays = GetDays(2078, 12, 31) - 1;
        Year = year;
        Month = month;
        Day = day;
        Days = GetDays(year, month, day);
    }
    private bool ValidateDate(int year, int month, int day)
    {
        bool valid = true;
        if (year < 1900 || year >= 2079)
            valid = false;
        else if (month < 1 || month > 12)
            valid = false;
        else if (day < 1 || day > 31)
            valid = false;
        else if (day > DaysPerMonth(year, month))
            valid = false;
        return valid;
    }
    private UInt32 GetDays(int year, int month, int day)
    {
        UInt32 days = 0;
        int yearSince1900 = year - 1900;
        UInt32 temp;
        for (int i = 1; i <= yearSince1900; i++)
        {
            temp = DaysPerYear(i + 1900 - 1);
            days += temp;
        }
        for (int i = 2; i <= month; i++)
        {
            temp = DaysPerMonth(year, i - 1);
            days += temp;
        }
        days += (UInt32)day;
        return days;
    }
    private bool IsLeap(int year)
    {
        return ((year % 4 == 0 && year % 100 != 0) || year % 400 == 0);
    }
    private UInt32 DaysPerYear(int year)
    {
        if (IsLeap(year))
            return 366;
        else
            return 365;
    }
    private UInt32 DaysPerMonth(int year, int month)
    {
        UInt32 days = (UInt32)MonthDay[month];
        if (month == 2 && !IsLeap(year))
            days = 28;
        return days;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I meant to use Uint16 for the backing field. That is why is limited to 1900-2078. \$\endgroup\$ – paparazzo Mar 29 '17 at 18:39
7
\$\begingroup\$

Properly overriding GetHashCode()

I remember when first learning how to properly override and use GetHashCode(), that there was a rule that stated The integer returned by GetHashCode() should never change. You're violating this rule because Days is not a readonly property.

Bad practices and unnecessary code

  1. Take a look at this snippet:
public int CompareTo(object obj)
{
    if (obj == null)
    {
        throw new ArgumentNullException();
    }
    if (obj is DateOnly)
    {
        DateOnly other = (DateOnly) obj;
        if (other == null)
        {
            throw new ArgumentNullException();
        }
        return (this.Days).CompareTo(other.Days);
    }
    else
    {
        throw new ArgumentNullException();
    }
}

There are 2 problems in this method.

First - you're using the is operator combined with a direct cast, instead of using the as operator.

Second - you have a redundant if statement that will never be triggered - if (other == null), if the cast (DateOnly other = (DateOnly) obj;) fails it will crash your program, it wont return null.

And lastly you don't really need an else statement there as there is flow branching involved if you trigger the if statement you will eventually reach the return statement which will break out of the method, thus not even continuing down your code, making the else redundant.

This is how I would write this method:

public int CompareTo(object obj)
{
    if (obj == null)
    {
        throw new ArgumentNullException();
    }
    var other = obj as DateOnly;
    if (other != null)
    {
        return (this.Days).CompareTo(other.Days);
    }
    throw new ArgumentNullException();
}
  1. In GetDayDiff(DateOnly Date) you perform 2 casts when only 1 is necessary to operate with the proper type:
long getDayDiff = (long) this.Days - (long) Date.Days;

You can just do:

long getDayDiff = this.Days - (long) Date.Days;
  1. I'm not sure why you inherit from object, this is unnecessary, as every type in C# inherits from that class whether it will be explicitly stated changes nothing, it's just few extra letters.

Shortening the code

  1. You can use interpolated strings instead of String.Format():

return string.Format("{0}/{1}/{2}", Year, Month, Day);

As interpolated string:

return $"{Year}/{Month}/{Day}";
  1. Ternary operator:

You can make use of the ternary operator here:

if (IsLeap(year))
    return 366;
else
    return 365;

Like this:

return (uint) (IsLeap(year) ? 366 : 365);

Overall design

Few concerns here:

  1. I would just use DateTime as a backing field and utilise some of the already written methods there (I'm sure this was a design pattern but I cant think of the name right now, maybe Decorator?).

  2. You lack range checks and overall validation of your public methods.

  3. You're inconsistent with where you use uint and int, if you've decided that it would be good to use uint because x, y, z, why are you not using that everywhere? This will also partially solve point 2.

\$\endgroup\$
  • \$\begingroup\$ How is get; private set; not read only? DateTime is 64 bites and don't need or want time or time zone here. Public method with a private set does not need a range check. Inherit from Object is so equal properties are equal. \$\endgroup\$ – paparazzo Mar 29 '17 at 3:19
  • 4
    \$\begingroup\$ @Paparazzi, private set and readonly properties share only 1 similarity, the place where you can set they're value is in the containing class itself, the difference is that you can set the value of a private set property anywhere you like in your class, where the readonly property can receive value only in the constructor or at initialization. I'm not sure what you mean by Inherit from Object is so equal properties are equal, inheriting from Object explicitly changes absolutely nothing, your class will always inherit from Object. \$\endgroup\$ – Denis Mar 29 '17 at 3:21
  • \$\begingroup\$ I thought you had to implement Object to override GetHashcode and Equals. \$\endgroup\$ – paparazzo Mar 29 '17 at 8:55
  • 1
    \$\begingroup\$ I am using Uint32 for Days to get the longest possible date range in 4 bytes. I made all the changes you suggested. Thanks Going to leave it up for 2 days before accepting an answer. \$\endgroup\$ – paparazzo Mar 29 '17 at 18:17
  • \$\begingroup\$ I messed up. I meant for the backing to be Uint16 but all your point are still valid. \$\endgroup\$ – paparazzo Mar 29 '17 at 18:31

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.