3
\$\begingroup\$

I need to test if a date is posterior to another in java, I just need to be sure that my approach is a good one

public class Date {
    private int day;
    private int month;
    private int year;

    public Date(int day, int month, int year) {
        this.day = day;
        this.month = month;
        this.year = year;
    }

    public int getDay() {
        return day;
    }

    public int getMonth() {
        return month;
    }

    public int getYear() {
        return year;
    }

    public boolean posterieurTo(Date dateToCompare){
        if (year > dateToCompare.getYear())
            return true;
        else if (month > dateToCompare.getMonth())
            return true;
        else if (day > dateToCompare.getDay())
            return true;
        else
            return false;
    }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ What is wrong with java.util.Date.before/after or java.util.Calendar.before/after or java.time.chrono.ChronoLocalDate.isBefore/isAfter? Or in other words: we already have 3 date/time apis already in the standard library, this is really nothing that needs another implementation. \$\endgroup\$ – mtj Feb 14 '17 at 6:52
5
\$\begingroup\$

Major problem: doesn't return correct value

The series of if statements you use is wrong because each smaller time unit should only be compared if the previous time unit is equal. You only check for greater than without checking for less than. So for example, if you compared "2016-02-14" with "2017-01-28", it would return true instead of false.

Here is a fixed version of your function:

public boolean posterieurTo(Date dateToCompare) {
    if (year > dateToCompare.getYear())
        return true;
    else if (year < dateToCompare.getYear())
        return false;

    if (month > dateToCompare.getMonth())
        return true;
    else if (month < dateToCompare.getMonth())
        return false;

    return (day > dateToCompare.getDay());
}

Alternative approach

Another way to go about comparing dates is to convert each date to single value and then compare those values. For example:

// Returns a monotonically increasing value.
private long getCompareValue() {
    return year * 366 + month * 31 + day;
}

public boolean posterieurTo(Date dateToCompare) {
    return getCompareValue() > dateToCompare.getCompareValue();
}
\$\endgroup\$
  • \$\begingroup\$ ... and the int part of the long value should be used as the hashCode() and there should be an equals() method too... and the posterieurTo should rely on a compareTo() method. \$\endgroup\$ – rolfl Feb 14 '17 at 11:45
5
\$\begingroup\$

The obvious

As mentioned in @mtj's comment, we have three different JDK APIs for doing this, so you really shouldn't have to resort to .

  1. java.util.Date.after(Date)
  2. java.util.Calendar.after(Object)
  3. java.time.LocalDate.isAfter(ChronoLocalDate)

Naming

Ok, maybe this is some simple assignment that you need to do... how about the naming? You should try to avoid using the same class name from the JDK, as it will get messy understanding which class is actually being used. For example, the wrong usage of your class might lead to confusing toString() representation, or syntax errors when trying to do a Date.getTime().

Also, since you are already using English mostly, you should translate that French-sounding method name to the English equivalent too, e.g. isAfter(). Whether you should have your entire codebase in non-English is another debating topic altogether. :)

Validation

You should be checking whether day and month are valid or not... you wouldn't want your bespoke class to start representing the 99th day of the 555th month of the 18288th year... assuming we're still talking about the Greogorian calendar.

null check

Next, assuming you have renamed this class, you should consider handling null properly: do you want to throw a NullPointerException as it is currently, or return a default value?

Boolean expressions

Finally, you can chain the boolean expressions together using && and || so that the method returns true the moment any of the expression is true too:

return dateToCompare != null && // assuming returning false for null values
        year > dateToCompare.getYear() || (year == dateToCompare.getYear() &&
            (month > dateToCompare.getMonth() || (month == dateToCompare.getMonth() && 
                day > dateToCompare.getDay())));

Admittedly, this is getting hard to comprehend.

(thanks to @JS1's answer for the update)

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