Day of year calculation method

Question

The exercise I wanted to solve is from here. Copying from that page:

public static int dayOfYear(int month, int dayOfMonth, int year) {
    if (month == 2) {
        dayOfMonth += 31;
    } else if (month == 3) {
        dayOfMonth += 59;
    } else if (month == 4) {
        dayOfMonth += 90;
    } else if (month == 5) {
        dayOfMonth += 31 + 28 + 31 + 30;
    } else if (month == 6) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31;
    } else if (month == 7) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30;
    } else if (month == 8) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31;
    } else if (month == 9) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31;
    } else if (month == 10) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30;
    } else if (month == 11) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31;
    } else if (month == 12) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 31;
    }
    return dayOfMonth;
}

This looks like a trivial example but it took me some time to follow the 'best practices' such as immutable objects, SOLID principles, etc.. And I am not sure if I was successful in following them.

I advise you to give you a try yourself actually!

And I wanted to refactor this "smelly" example, and here is my implementation in Java.

Months are hardcoded with number of days they have:

public enum Month {

    JAN(1, 31, 31),
    FEB(2, 28, 29),
    MAR(3, 31, 31),
    APR(4, 30, 30),
    MAY(5, 31, 31),
    JUN(6, 30, 30),
    JUL(7, 31, 31),
    AUG(8, 31, 31),
    SEP(9, 30, 30),
    OCT(10, 31, 31),
    NOV(11, 30, 30),
    DEC(12, 31, 31);

    private final int monthIndex;
    private final int numberOfDaysInNonLeapYear;
    private final int numberOfDaysInLeapYear;

    Month(int monthIndex, int numberOfDaysInNonLeapYear, int numberOfDaysInLeapYear) {
        this.monthIndex = monthIndex;
        this.numberOfDaysInNonLeapYear = numberOfDaysInNonLeapYear;
        this.numberOfDaysInLeapYear = numberOfDaysInLeapYear;
    }

    public int getNumberOfDaysInNonLeapYear() {
        return numberOfDaysInNonLeapYear;
    }

    public int getNumberOfDaysInLeapYear() {
        return numberOfDaysInLeapYear;
    }

    public int getMonthIndex() {
        return monthIndex;
    }

    static Month getMonthForIndex(final int index) {
        final Month[] months = Month.values();
        for (final Month month : months) {
            if (month.monthIndex == index) {
                return month;
            }
        }
        return null;
    }
}

A Month has more meaning when it is a YearMonth:

public class YearMonth {

    private final Month month;
    private final boolean isLeapYear;
    private final int yearValue;

    public YearMonth(final int yearValue, final Month month) {
        this.yearValue = yearValue;
        this.month = month;
        if (yearValue % 4 == 0) {
            isLeapYear = true;
        } else {
            isLeapYear = false;
        }
    }

    public final int numberOfDays() {
        if (isLeapYear) {
            return month.getNumberOfDaysInLeapYear();
        }
        return month.getNumberOfDaysInNonLeapYear();
    }

    public final int dayOfYear(final int dayOfMonth) {
        int dayOfYear = 0;
        int monthIndex = month.getMonthIndex() - 1;

        while (monthIndex != 0) {
            final Month previousMonth = Month.getMonthForIndex(monthIndex);
            final YearMonth yearPreviousMonth = new YearMonth(yearValue, previousMonth);
            dayOfYear = dayOfYear + yearPreviousMonth.numberOfDays();
            monthIndex--;
        }

        return dayOfYear + dayOfMonth;
    }
}

Two sample test cases:

public class TestClass {
    public static void main(String[] args) {
        final Month january = Month.getMonthForIndex(1);
        final YearMonth january2016 = new YearMonth(2016, january);
        final int dayOfYearJanuary2016Day1 = january2016.dayOfYear(1);
        System.out.println("Day of Year 2016 for January Day 1: " + dayOfYearJanuary2016Day1);

        final Month march = Month.getMonthForIndex(3);
        final YearMonth march2014 = new YearMonth(2014, march);
        final int dayOfYearMarch2014Day2 = march2014.dayOfYear(2);
        System.out.println("Day of Year 2014 for March Day 2: " + dayOfYearMarch2014Day2);
    }
}

Output will be:

Day of Year 2016 for January Day 1: 1
Day of Year 2014 for March Day 2: 61

Any comments on code smells or fragility are welcome.

Answer 1

As has been said, there are shorter ways to achieve a similar goal. However, lets have a look at your actual code.

Storing Leap Days

Your storing more information for each month than you really need to:

JAN(1, 31, 31)

For 11 out of 12 months, the leap and normal year are going to have the same value. Since you are already building an enum with functionality it feels like you only really need to store the normal number of days in the month. You then simply adjust your calculation if the month is February:

public int getNumberOfDaysInLeapYear() {
    return numberOfDaysInNonLeapYear + this == FEB ? 1 : 0;
}

getMonthForIndex

Your getMonthForIndex has a bit of a strange name. When I think about months in dates, I don't think about them in terms of index. I might think of the months number, but naming the method getMonth seems more intuitive. As it stands, the method loops through and checks the index of the month each time. Since the enums values are in the order declared, you don't need to do this, you can simply index into the values. You are also returning null if an invalid index is requested. Again, this feels wrong, if you know the argument is wrong, throw an exception. So you might end up with something closer to this:

static Month getMonth(final int index) {
    final Month[] months = Month.values();
    if(index < 1 || index > months.length+1) {
        throw new IllegalArgumentException("index out of month range");
    }
    return months[index-1];
}

getNumberOfDaysInXXX

I find the getNumberOfDaysInLeapYear and getNumberOfDaysInNonLeapYear to be a bit superfluous. I'd prefer to have a getNumberOfDays method that takes a flag to indicate if it's a leap year or not.

Month

Putting the changes together, you might end up with a Month enum that looks like this (although I'm not sure that the monthOfYear field is actually needed):

public enum Month {

        JAN(1, 31),
        FEB(2, 28),
        MAR(3, 31),
        APR(4, 30),
        MAY(5, 31),
        JUN(6, 30),
        JUL(7, 31),
        AUG(8, 31),
        SEP(9, 30),
        OCT(10, 31),
        NOV(11, 30),
        DEC(12, 31);

        private final int monthOfYear;
        private final int numberOfDaysInMonth;

        Month(int monthIndex, int numberOfDaysInMonth) {
            this.monthOfYear = monthIndex;
            this.numberOfDaysInMonth = numberOfDaysInMonth;
        }

        public int getNumberOfDays() {
            return getNumberOfDays(false);
        }

        public int getNumberOfDays(boolean isLeapYear) {
            return isLeapYear && this == FEB ? numberOfDaysInMonth + 1 : numberOfDaysInMonth;
        }

        public int getMonthOfYear() {
            return monthOfYear;
        }

        static Month getMonth(final int index) {
            final Month[] months = Month.values();
            if(index < 1 || index > months.length+1) {
                throw new IllegalArgumentException("index out of month range");
            }
            return months[index-1];
        }
}

YearMonth or Year

I'd don't really like this classes name. If you look at the operations it's doing, I think that really it should be a Year class. It calculates if it is a leap year, works out the day of the year etc.

dayOfYear

The signature for this method is not intuitive. dayOfYear(dayOfMonth) what does dayOfMonth mean?!? It creates lots of instances of itself in order to perform the calculation, all of which feels wrong. If the class has been modified to be Year, then the month would passed into it. This gives a method signature of:

public final int dayOfYear(final Month month, final int dayOfMonth)

Which makes more sense (at least to me), I can see how dayOfMonth is related to the call. You can also take advantage of the fact that you're working with an enum to iterate over the values.

Year

Putting the above together, you might end up with the Year class that looks more like this:

public class Year {

    private final boolean isLeapYear;

    public Year(final int yearValue) {
        this.isLeapYear = isLeapYear(yearValue);
    }

    private static boolean isLeapYear(final int year) {
        if (year % 4 == 0 && (year%100 != 0 || year%400 == 0)) {
            return true;
        } else {
            return false;
        }       
    }
    public final int dayOfYear(final Month month, final int dayOfMonth) {
        int dayOfYear = 0;

        for(Month iter : Month.values()){
            if(iter == month) break;
            dayOfYear += iter.getNumberOfDays(isLeapYear);
        }

        return dayOfYear + dayOfMonth;
    }
}

TestClass

Visually inspecting your output for your test cases is OK, for simple stuff. However, where you're doing something like refactoring it's often better to write unit tests. You can then validate that the before code and the after code perform the same. So you might end up with some JUnit tests like:

@Test
public void firstMonthShouldBeJanuary() {
        Month month = Month.getMonth(1);
        assertEquals(Month.JAN, month);
}

@Test
public void FebShouldBe28Days() {
    Month month = Month.FEB;
    assertEquals(28, month.getNumberOfDays());      
}

@Test
public void LeapYearFebShouldBe29Days() {
    Month month = Month.FEB;
    assertEquals(29, month.getNumberOfDays(true));
}

@Test
public void dayOneOfJanuary2016ShouldBeFirstOfYear() {

    final Month january = Month.getMonth(1);
    final Year january2016 = new Year(2016);

    assertEquals(1, january2016.dayOfYear(january, 1));
}    

@Test
public void secondOfMarch2014ShouldBe61stDayOfYear() {
    final Month march = Month.getMonth(3);
    final Year march2014 = new Year(2014);
    assertEquals(61, march2014.dayOfYear(march, 2));
}

@Test
public void secondOfMarchOnLeapYearShouldBe62ndDayOfYear() {
    final Month march = Month.getMonth(3);
    final Year march2014 = new Year(2012);
    assertEquals(62, march2014.dayOfYear(march, 2));
}

Answer 2

Your code is way too bloated. The definitions for leap years will not change in the foreseeable future, so this simple utility method does not need to apply to changing requirements.

private static final int[] DAYS_BEFORE = {0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334};

public static int dayOfYear(int month, int dayOfMonth, int year) {
    int leapDays = month > 2 && (year % 4 == 0 && (year % 100 != 0 || year % 400 == 0)) ? 1 : 0;
    return DAYS_BEFORE[month - 1] + dayOfMonth + leapDays;
}

This is all that is needed. 6 lines instead of 84. So when there should ever be a bug in the code, it will be easy to find, since there are only 3 lines where it could hide.

Answer 3

Your leap year calculation is wrong:

if (yearValue % 4 == 0)

You're not accounting for variations on centuries. Years divisible by 100 aren't leap, unless they're also divisible by 400.

Answer 4

You can use switch fallthrough to accomplish your original algorithm more elegantly.

switch (month) {
case 12:
    dayOfMonth += 31;
case 11:
    dayOfMonth += 31;
case 10:
    dayOfMonth += 30;
case 9:
    dayOfMonth += 31;
case 8:
    dayOfMonth += 31;
case 7:
    dayOfMonth += 30;
case 6:
    dayOfMonth += 31;
case 5:
    dayOfMonth += 30;
case 4:
    dayOfMonth += 31;
case 3:
    dayOfMonth += 28;
case 2:
    dayOfMonth += 31;
}

Without the break at the end of each case, control will execute the statement within it and every statement below it. So 5 will first execute dayOfMonth += 30, then 31, 28, and 31 as you desire. It's just happening in reverse.