2
\$\begingroup\$

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());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you required to write this yourself, or can you use SimpleDateFormat with setLenient(false)? \$\endgroup\$ – Eric Stein Apr 1 '15 at 14:26
  • \$\begingroup\$ @EricStein I could, but in order to learn java properly, I'd like to try and implement this \$\endgroup\$ – Nilzone- Apr 1 '15 at 14:29
  • \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success Apr 2 '15 at 17:48
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Lots of great points and suggestions - Thank you! About the enum, I agree right now it doesn't serve much of a purpose besides populating the map which could easily be done without enum. But say I want to keep it an enum, what type of methods should be added, so that my choice to use enum would be justified? Maybe a silly question, but until you pointed it out, I thought enum was a good choice to handle the month stuff.. \$\endgroup\$ – Nilzone- Apr 1 '15 at 15:10
  • \$\begingroup\$ @Nilzone- Hard to say what would make the enum worthwhile. I guess if you extend the class to perform other functions it may be useful. \$\endgroup\$ – Duncan Jones Apr 1 '15 at 18:39

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.