3
\$\begingroup\$

I am writing some date utility methods for a project. One of the methods will answer the following questions (for example):

'When is the (3rd) (Monday) of (February) in the year (2018)'
'When is the (1st) (Wednesday) of (October) in the year (2020)'

The method accepts the 4 int value parameters as arguments, and will return a Date.

Here is the method as written:

/**
 * Example usage:
 * find the 3rd Monday of February, in the year 2018 getDayOfMonth(3, Calendar.MONDAY, Calendar.FEBRUARY, 2018)
 * find the 1st Tuesday of October, in the year 2017 getDayOfMOnth(1, Calendar.TUESDAY, Calendar.OCTOBER, 2017)
 * @param n occurrence count
 * @param dayOfWeek day of week to find
 * @param month month to use
 * @param year year to use
 * @return Date that the nth dayOfWeek occurs
 */
public static Date getDate(int n, int dayOfWeek, int month, int year){
    Calendar calendar = Calendar.getInstance();
    calendar.set(Calendar.YEAR, year);
    calendar.set(Calendar.MONTH, month);
    calendar.set(Calendar.DAY_OF_MONTH, 1);

    int matchCount = 0;

    while(true){
        if(calendar.get(Calendar.DAY_OF_WEEK) == dayOfWeek) {
            matchCount++;
        }
        if(matchCount == n){
            break;
        }
        calendar.add(Calendar.DATE, 1);
        if(calendar.get(Calendar.MONTH) != month){
            break;
        }
    }

    if(matchCount != n){
        throw new RuntimeException("error");
    }

    return calendar.getTime();
}

I am struggling to come up with a good name for the method and the 1st parameter.

\$\endgroup\$
2
\$\begingroup\$

For the parameter, may I suggest nthOccurrence? Alternatively, anything that communicates what exactly it means should be alright, given that it's at a pretty contained location - even nthTimeDayOfWeekOccursInMonth wouldn't be terrible, especially since dayOfWeek and month are also parameters.

The method is harder, mainly because the thing you're calculating doesn't have a great name. "Return the nth occurrence of a weekday in a given month of a given year" doesn't lend itself to brevity. I would take one of two routes:

Does the assignment / class / professor have a term for this functionality? "Weekday oracle" or something equally obscure works perfectly if it's the way that everyone else who will see this code knows to refer to this thing.

If not, I would name it what it is; something like getDateOfNthTimeDayOfWeekOccursInMonth - this would pair well with naming that first parameter nthOccurrence. In this case, I think information (what exactly can I expect this to do) trumps brevity, because what it does is not very common / intuitive.

\$\endgroup\$
1
\$\begingroup\$

In addition to @MyStackRunnethOver notes, here is what I think you can improve:

Parameters validation

First of all, the function does not check the validity of any of its input. This may raise bugs in your application. A practical case is when the user types 1 to refer to January and ends up by swimming in February because the first month of the year is JANUARY which is 0.

while() loop optimization

In case we set n to be 29, for instance, the while() loop will iterate that much. You can easily optimize that by early checking that n is within a set of (1, 2, 3, 4, 5) because 5 is the maximum number of times a day can re-appear within the same month.

Think of persistence

In case one day you want to use this utility to save dates into a database, then may be you should deal with the Gregorian calendar (Calendar calendar = GregorianCalendar.getInstance();) instead as it is the commonly used by SGBDR.

\$\endgroup\$
0
\$\begingroup\$

Less iteration

public static Date getDate(int n, int dayOfWeek, int month, int year){
    Calendar calendar = Calendar.getInstance();
    calendar.set(Calendar.YEAR, year);
    calendar.set(Calendar.MONTH, month);
    calendar.set(Calendar.DAY_OF_MONTH, 1);

    int matchCount = 0;

    while(true){
        if(calendar.get(Calendar.DAY_OF_WEEK) == dayOfWeek) {
            matchCount++;
        }
        if(matchCount == n){
            break;
        }
        calendar.add(Calendar.DATE, 1);
        if(calendar.get(Calendar.MONTH) != month){
            break;
        }
    }

    if(matchCount != n){
        throw new RuntimeException("error");
    }

    return calendar.getTime();
}

Consider

public static Date calculateDateFor(int ordinal, int dayOfWeek, int month, int year) {
    Calendar calendar = Calendar.getInstance();
    calendar.set(Calendar.YEAR, year);
    calendar.set(Calendar.MONTH, month);
    calendar.set(Calendar.DAY_OF_MONTH, 1);

    while (calendar.get(Calendar.DAY_OF_WEEK) != dayOfWeek) {
        calendar.add(Calendar.DATE, 1);
    }

    for (int weekCount = 1; weekCount < ordinal; weekCount++) {
        calendar.add(Calendar.DATE, 7);
    }

    if (calendar.get(Calendar.MONTH) != month || ordinal <= 0) {
        throw new RuntimeException("error");
    }

    return calendar.getTime();
}

To me, a method named get returns a field from the object or class. This doesn't. Instead, it calculates a date for a particular input. I would find calculateDateFor sufficient, but you can write out something like calculateDateForOrdinalDayOfWeekInMonthYear if you prefer.

The nth something is the ordinal number or ordinal for short. You may of course prefer ordinalNumber.

In your original, you iterate by one each time. However, once on the right day of the week, you can iterate by seven.

The name matchCount doesn't indicate what is being matched. So I changed it to weekCount instead.

You check, after the fact, that the ordinal value was not too large. But you don't check that it isn't too small. I added that check to the exception gate. The original behavior was to print the first day of the month whenever the ordinal value is less than or equal to zero. This version changes that to be an exception instead.

With math

You use loops here, but that's not necessary. It's possible to get the same effect with just math.

public static final int DAY_COUNT_PER_WEEK = 7;

public static Date calculateDateFor(int ordinal, int dayOfWeek, int month, int year) {
    Calendar calendar = Calendar.getInstance();
    calendar.set(Calendar.YEAR, year);
    calendar.set(Calendar.MONTH, month);
    calendar.set(Calendar.DAY_OF_MONTH, 1);

    int dayCountTo = dayOfWeek - calendar.get(Calendar.DAY_OF_WEEK);

    // if the day of the week sought is before the day of the week
    // of the first day of the month, we need to add a week
    if (dayCountTo < 0) {
        dayCountTo += DAY_COUNT_PER_WEEK;
    }

    dayCountTo += DAY_COUNT_PER_WEEK * (ordinal - 1);

    calendar.add(Calendar.DATE, dayCountTo);

    if (calendar.get(Calendar.MONTH) != month) {
        throw new RuntimeException("error");
    }

    return calendar.getTime();
}

Note that now if the ordinal number is too small or too large, it will produce an invalid month and trigger the exception. So we can remove the explicit check on ordinal.

Alternately, consider

public static Date calculateDateFor(int ordinal, int dayOfWeek, int month, int year) {
    Calendar calendar = Calendar.getInstance();
    calendar.set(Calendar.YEAR, year);
    calendar.set(Calendar.MONTH, month);
    calendar.set(Calendar.DAY_OF_MONTH, 1);

    int dayCountTo = dayOfWeek - calendar.get(Calendar.DAY_OF_WEEK);
    calendar.add(Calendar.DATE, dayCountTo);

    // if the day of the week sought is before the day of the week
    // of the first day of the month, we need to add a week
    // but ordinal is not zero-indexed, so we have to subtract a week
    // net result is to sometimes use ordinal and otherwise ordinal - 1
    int weekCountTo = (dayCountTo < 0) ? ordinal : (ordinal - 1);

    calendar.add(Calendar.WEEK_OF_MONTH, weekCountTo);

    if (calendar.get(Calendar.MONTH) != month) {
        throw new RuntimeException("error3");
    }

    return calendar.getTime();
}

This avoids manually setting the number of days in the week and relies on Calendar to perform the math.

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