2
\$\begingroup\$

I implemented a Date based on the Gregorian calendar as an exercise. I'm looking mainly for how to make it more readable and easy to test, but any feedback is welcome.

public interface Date {
    GregorianDate addDays(final long offset);
    boolean isLeapYear();
    int getYearDays();
    DayOfWeek getDayOfWeek();

    public static Date of(long year, Month month, int day) {
        return new GregorianDate(year, month, day);
    }
}

public final class GregorianDate implements Date {
    private static final int DAYS_IN_YEAR_NO_LEAP = 365;
    private static final long LEAP_YEAR_INTERVAL  = 4;
    private static final long CENTURY_INTERVAL    = 4;
    private static final long CENTURY             = 100;
    private static final long CENTURY_OFFSET      = 5;

    private final int day;
    private final Month month;
    private final long year;

    public GregorianDate(final long year, final Month month, final int day) {
        if(!isValidDate(year, month, day)) {
            throw new IllegalArgumentException("Invalid date");
        }
        this.day   = day;
        this.month = month;
        this.year  = year;
    }

    private boolean isValidDate(long year, Month month, int day) {
        int leap = month == Month.FEBRUARY ? leapCount(year) : 0;
        return 0 < day &&  day <= (month.getDays() + leap);
    }

    public static boolean isLeapYear(final long year) {
        return (year % LEAP_YEAR_INTERVAL == 0 && year % CENTURY != 0)
                || year % (CENTURY_INTERVAL * CENTURY) == 0;
    }

    public boolean isLeapYear() {
        return isLeapYear(year);
    }

    public GregorianDate addDays(final long offset) {
        long yearDays = Math.addExact(getYearDays(), offset);
        long year     = this.year;

        while (yearDays > daysInYear(year)) {
            yearDays -= daysInYear(year);
            year = Math.incrementExact(year);
        }
        while(yearDays <= 0) {
            year = Math.decrementExact(year);
            yearDays += daysInYear(year);
        }
        return fromYearDays((int) yearDays, year);
    }

    private GregorianDate fromYearDays(final int daysOfYear, final long year) {
        Month month = Month.DECEMBER;
        int leap    = leapCount(year);

        while(daysOfYear <= (month.getAccumulatedDays() + leap)) {
            month = month.previous();
            leap  = !greaterThanFebruary(month) ? 0 : leap;
        }
        int days = daysOfYear
                - month.getAccumulatedDays()
                - leap;
        return new GregorianDate(year, month, days);
    }

    private int daysInYear(final long year) {
        return DAYS_IN_YEAR_NO_LEAP + leapCount(year);
    }

    public int getYearDays() {
        int days =  month.getAccumulatedDays() + day;
        if (greaterThanFebruary(month)) {
            days += leapCount(year);
        }
        return  days;
    }

    public DayOfWeek getDayOfWeek() {
        final long yearCopy = year - (!greaterThanFebruary(month) ? 1 : 0);
        final long century  = Math.floorDiv(yearCopy, CENTURY);
        final long week = (day
                + cycleOffset(century)
                + monthOffset(month)
                + centuryReminderOffset(yearCopy))
                % DayOfWeek.daysInWeek();

        return DayOfWeek.ofIndex((int) week);
    }

    private static int leapCount(final long year) {
        return isLeapYear(year) ? 1 : 0;
    }

    private static boolean greaterThanFebruary(final Month month) {
        return month.compareTo(Month.FEBRUARY) > 0;
    }

    private long cycleOffset(final long century) {
        return CENTURY_OFFSET * Math.floorMod(century, CENTURY_INTERVAL);
    }

    private long centuryReminderOffset(final long dateYear) {
        long centuryRem       = Math.floorMod(dateYear , CENTURY);
        long centuryLeapYears = (centuryRem / LEAP_YEAR_INTERVAL);
        return centuryRem + centuryLeapYears;
    }

    private int monthOffset(final Month month) {
        return month.getAccumulatedDays()
                - (greaterThanFebruary(month) ? 1 : 0);
    }

    @Override
    public String toString() {
        return String.format("(%d, %s, %d)",
                day, month.toNumber(), year);
    }
}

public enum DayOfWeek {
    SUNDAY,
    MONDAY,
    TUESDAY,
    WEDNESDAY,
    THURSDAY,
    FRIDAY,
    SATURDAY;

    private static final DayOfWeek[] DAYS_IN_WEEK = DayOfWeek.values();
    private static final int MAX_INDEX = 6;
    private static final int MIN_INDEX = 0;


    public static DayOfWeek ofIndex(final int index) {
        if (index < MIN_INDEX || index > MAX_INDEX) {
            throw new IllegalArgumentException();
        }
        return DAYS_IN_WEEK[index];
    }

    public static int daysInWeek() {
        return DAYS_IN_WEEK.length;
    }
}

public enum Month {
    JANUARY,
    FEBRUARY,
    MARCH,
    APRIL,
    MAY,
    JUNE,
    JULY,
    AUGUST,
    SEPTEMBER,
    OCTOBER,
    NOVEMBER,
    DECEMBER;


    private static final int[] ACCUMULATED_DAYS = {
            0, 31, 59, 90,
            120, 151, 181,
            212, 243, 273,
            304, 334, 365
    };
    private static final Month[] MONTHS = Month.values();

    public int getDays() {
        int index = ordinal();
        return ACCUMULATED_DAYS[index + 1]
                - ACCUMULATED_DAYS[index];
    }

    public int getAccumulatedDays() {
        return ACCUMULATED_DAYS[ordinal()];
    }

    public int toNumber() {
        return ordinal() + 1;
    }

    public static Month fromNumber(final int number) {
        if( MONTHS.length < number || number < 1 ) {
            throw new IllegalArgumentException();
        }
        return MONTHS[number - 1];
    }

    public Month offset(final int offset) {
        return MONTHS[Math.floorMod(this.ordinal() + offset, MONTHS.length)];
    }

    public Month previous() {
        return offset(-1);
    }

    public Month next() {
        return offset(1);
    }
}
\$\endgroup\$
  • \$\begingroup\$ make a static utility method monthLength(year, month), it would be useful, and also simplifies the implementation of isValidDate \$\endgroup\$ – Mark Jeronimus Jun 7 '18 at 8:40
1
\$\begingroup\$

Consistent naming

    private static final long CENTURY_INTERVAL    = 4;
    private static final long CENTURY             = 100;

I don't like these names. They're not consistent with LEAP_YEAR_INTERVAL. Consider

    private static final long CENTURY_INTERVAL      = 100;
    private static final long LEAP_CENTURY_INTERVAL = 4 * CENTURY_INTERVAL;

Now it's easier to follow what's happening. And later we'll save some math.

        return (year % LEAP_YEAR_INTERVAL == 0 && year % CENTURY != 0)
                || year % (CENTURY_INTERVAL * CENTURY) == 0;

Instead,

        return (year % LEAP_YEAR_INTERVAL == 0 && year % CENTURY_INTERVAL != 0)
                || year % LEAP_CENTURY_INTERVAL == 0;

I'd rather do math once in the constant definition than rely on the compiler to cache it into once.

Efficiency

Also consider writing it out more efficiently as

        if (year % LEAP_YEAR_INTERVAL != 0) {
            return false;
        }

        // although every century is divisible by four,
        // most (e.g. 1900 and 2100) are not leap years
        // every fourth century is (e.g. 2000)
        return year % CENTURY_INTERVAL != 0 || year % LEAP_CENTURY_INTERVAL == 0;

This way as soon as we find out that it is not a leap year or is a leap year, we return. So 75% of the time, it will never get past the first if statement. And 24% of the time, it will not bother to check if it is a leap century (because it's not a century). It will only do the full check 1% of the time.

The one statement version always does two comparisons and occasionally (1%) does three. So it always does at least as many comparisons and usually does more (75% of the time).

I added a descriptive comment, as I don't find these rules obvious. I didn't try to explain what I was doing, as the code should be readable on its own. I explained why.

Enum construction

public enum Month {
    JANUARY,
    FEBRUARY,
    MARCH,
    APRIL,
    MAY,
    JUNE,
    JULY,
    AUGUST,
    SEPTEMBER,
    OCTOBER,
    NOVEMBER,
    DECEMBER;


    private static final int[] ACCUMULATED_DAYS = {
            0, 31, 59, 90,
            120, 151, 181,
            212, 243, 273,
            304, 334, 365
    };
    private static final Month[] MONTHS = Month.values();

    public int getDays() {
        int index = ordinal();
        return ACCUMULATED_DAYS[index + 1]
                - ACCUMULATED_DAYS[index];
    }

    public int getAccumulatedDays() {
        return ACCUMULATED_DAYS[ordinal()];
    }

    public int toNumber() {
        return ordinal() + 1;
    }

    public static Month fromNumber(final int number) {
        if( MONTHS.length < number || number < 1 ) {
            throw new IllegalArgumentException();
        }
        return MONTHS[number - 1];
    }

You can do this without maintaining the ACCUMULATED_DAYS separately. Consider

public enum Month {

    JANUARY(0, 31),
    FEBRUARY(31, 28),
    MARCH(59, 31),
    APRIL(90, 30),
    MAY(120, 31),
    JUNE(151, 30),
    JULY(181, 31),
    AUGUST(212, 31),
    SEPTEMBER(243, 30),
    OCTOBER(273, 31),
    NOVEMBER(304, 30),
    DECEMBER(334, 31);

    private static final Month[] MONTHS = Month.values();

    private final int accumulatedCount;
    private final int dayCount;

    Month(int accumulatedCount, int dayCount) {
        this.accumulatedCount = accumulatedCount;
        this.dayCount = dayCount;
    }

    public int getDayCount() {
        return dayCount;
    }

    public int getAccumulatedDays() {
        return accumulatedCount;
    }

    public int toNumber() {
        return ordinal() + 1;
    }

    public static Month fromNumber(final int number) {
        if( MONTHS.length < number || number < 1 ) {
            throw new IllegalArgumentException();
        }

        return MONTHS[number - 1];
    }

Now the two get methods do what one would expect. They return an object field. An alternative to getDayCount would be

    public static final int YEAR_DAY_COUNT = 365;

    public calculateDayCount() {
        int accumulatedAtEnd = next().getAccumulatedDays();
        if (accumulatedAtEnd == 0) {
            accumulated = YEAR_DAY_COUNT;
        }

        return accumulatedAtEnd - getAccumulatedDayCount();
    }

This would ensure that the accumulated days match the days of each month. But I actually think that that would be overkill. Rather than doing things that way, I think it's better to do things the first way and then use something like calculateDayCount in a test method to verify that the amounts match. The reason is that calculating the days from the accumulation is exactly backwards. The number of days in the month is the primary attribute. The accumulated days is the secondary attribute, calculated from the number of days in each month and the month order.

It's possible to build the accumulatedCount.

    static {
        int accumulatedCount = 0;
        for (Month month : MONTHS) {
            month.accumulatedCount = accumulatedCount;
            accumulatedCount += month.getDayCount();
        }
    }

    private int accumulatedCount;

But to do it, we have to take the final modifier off accumulatedCount.

Testing

Overall, I think that I would prefer to keep it and initialize manually. To verify that I didn't make a mistake, we could test:

    public static final int YEAR_DAY_COUNT = 365;

    public calculateDayCount(Month month) {
        int accumulatedAtEnd = month.next().getAccumulatedCount();
        if (accumulatedAtEnd == 0) {
            accumulated = YEAR_DAY_COUNT;
        }

        return accumulatedAtEnd - month.getAccumulatedCount();
    }

    testAccumulatedDaysInMonths() {
        for (Month month : Month.values()) {
            assertEquals(month.getDays(), calculateDayCount(month));
        }
    }

Now we can keep the final property, indicating that once set, these values never change. We still have to do some calculation outside the program that is subject to error, but we can test that it is correct.

While I implemented this with JUnit, I would argue that this is technically an integration test rather than a unit test. It's checking whether two values (for each month) are set consistently with each other, not whether either is calculated correctly.

\$\endgroup\$
  • \$\begingroup\$ would the if be preferable to year % LEAP_YEAR_INTERVAL == 0 && (year % CENTURY_INTERVAL != 0 || year % LEAP_CENTURY_INTERVAL == 0) with some proper use of new lines? \$\endgroup\$ – MAG Jun 4 '18 at 2:37
2
\$\begingroup\$

As a fellow programmer, I encourage you to replace

@Override
public String toString() {
    return String.format("(%d, %s, %d)", day, month.toNumber(), year);
}

by

@Override
public String toString() {
    return year + "-" + month.toNumber() + "-" + day;
}

To be ISO 8601 compliant and developer-friendly.

Since your constructor takes the year, month and day in that same order, it's in your interest to be consistent in your toString() method as well.

\$\endgroup\$
  • \$\begingroup\$ according to the link, this format is not ISO 8601 compliant \$\endgroup\$ – Sharon Ben Asher Jun 4 '18 at 6:58
  • \$\begingroup\$ YYYY-MM-DD is ISO 8601 compliant. \$\endgroup\$ – TwiN Jun 4 '18 at 11:15
  • \$\begingroup\$ ...but this is not the format in the code \$\endgroup\$ – Sharon Ben Asher Jun 4 '18 at 11:20
  • \$\begingroup\$ Enlighten me. In the current code, the date components are ordered as such: day, month, year. In the second code snippet, year, month, day. While I didn't test the code myself, I'd expect the code snippet I provided to result in YYYY-MM-DD, though MM and DD won't be 0 padded. \$\endgroup\$ – TwiN Jun 4 '18 at 11:23
  • \$\begingroup\$ look at the literal part I see parenthesis and commas ... \$\endgroup\$ – Sharon Ben Asher Jun 4 '18 at 12:27
0
\$\begingroup\$

regarding String formatting: If your concern is readability, then this is fine. However, you need to be aware it comes with performance cost. The cost comes from parsing the format pattern. and this will occur every time the method is called and it is not unlikely that toString() is called in a loop (as in printing the contents of a collection) so from performance POV, string concatanation is a better solution.

\$\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.