Date validation in Java

Question

I'm learning Java, and would really appreciate any tips on how to improve this code.

It's just a simple date validation class.

import java.util.Map;
import java.util.HashMap;

class DateValidator {
    private int day;
    private int month;
    private int year;

    private enum Months {
        JANUARY(1, 31), FEBRUARY(2, 28), MARCH(3, 31), APRIL(4, 30), MAY(5, 31), JUNE(6, 30), 
        JULY(7, 31), AUGUST(8, 31), SEPTEMBER(9, 30), OCTOBER(10, 31), NOVEMBER(11, 30), DECEMBER(12, 31);

        private final int MonthNumber;
        private final int numberOfDaysInMonth;

        private static Map<Integer, Integer> map;

        private Months(int monthMnumber, int numberOfDays) {
            this.MonthNumber = monthMnumber;
            this.numberOfDaysInMonth = numberOfDays;
        }

        static {
            map = new HashMap<Integer, Integer>();

            for (Months m : Months.values()) {
                map.put(m.MonthNumber, m.numberOfDaysInMonth);
            }
        }

        public static Map<Integer, Integer> getMap() {
            return map;
        }

    }   

    DateValidator(int d, int m, int y) {
            this.day = d;
            this.month = m;
            this.year = y;
    }

    private static boolean leapYear(int y) {
        return ((y % 4 == 0) && (y % 100 != 0) || (y % 400 == 0));
    }

    public boolean checkDay() {
        if (this.month == 2 && this.leapYear(this.year)) {
            Months.getMap().put(this.month, 29);
        }

        return (this.day <= Months.getMap().get(this.month));
    }
}

class Main {
    public static void main(String[] args) {
        DateValidator d = new DateValidator(31, 10, 90);

        System.out.print(d.checkDay());
    }
}

Answer 1

A few observations in no particular order:

• I would consider redesigning the class so that it presents a static method: checkDay(day, month, year). There doesn't seem to be much benefit in requiring callers to create an instance of the class, bound to a particular date. If you do this, ensure you make your constructor private so that the class cannot be instantiated.

• If you decide to stick with the current layout, then the fields day, month and year could be declared as final, since you don't adjust them at all within the class.

• A minor point, but your constructor may be more readable if you use day, month and year rather than d, m and y.

• Because your leapYear method is static, you should call it in a static way:

  public boolean checkDay() {
    if (this.month == 2 && leapYear(this.year)) { // <----- here
      Months.getMap().put(this.month, 29);
    }
  

• In general, you use this. more than is necessary. I guess it's a style issue, but generally code will only use this. when it's necessary to distinguish between local variables of the same name and class fields.

• In your Months enum, you have a field named MonthNumber, but it should be monthNumber to match normal Java style.

• You've created an enum for your month data, however this is used solely to populate a map. The rest of your code just uses the map. As a result, I would suggest you remove the enum and just stick with a map, unless you plan to extend this class in future and use the enum for other purposes. If you stick with using an enum, the map can be declared final.

• It would be easier to read your leapYear method if you added some parentheses, rather than relying solely on operator precedence. You don't need the outer parentheses however:

  return (y % 4 == 0) && ((y % 100 != 0) || (y % 400 == 0));
  

• In your checkDay() method, I would avoid putting data into your map. Certainly you don't want to pollute your map if you plan to move to a static checkDay() method as discussed above. I would do something like this instead:

  public boolean checkDay() {
    int daysInMonth =
        month == 2 && leapYear(year) ? 29 : Months.getMap().get(month);
  
    return day <= daysInMonth;
  }
  

• (Very minor...) Typically one would call your method isLeapYear rather than simply leapYear.