4
\$\begingroup\$

I'd really appreciate some general formatting/style/performance tips, as well as any ways I could improve this code in general that I wrote for an assignment (title) that's due at the end of the week. The code passes every test input given (leap years and such), so the logic is all set. I had the HashSet as a List originally, but I read that a HashSet will give better performance, although I'm not sure if it even matters with only 4 values.

import java.util.*;

class Dates {

    private static final Set<Integer> THIRTY_DAY_MONTHS = new HashSet<>(Arrays.asList(4, 6, 8, 11));
    private String monthString;

    private Dates() {
        //Initializing input method
        System.out.println("Enter month/day/year to be converted to normal format: ");
        Scanner s = new Scanner(System.in);

        //Creates an array of strings split at forward slash
        String[] input = s.nextLine().split("/");

        //Converts the array of strings to array of ints with matching indices
        int[] inputNumbers = new int[input.length];
        for (int i = 0; i < input.length; i++) {
            inputNumbers[i] = Integer.parseInt(input[i]);
        }

        //String month values for numeric inputs
        if (inputNumbers[0] == 1) {
            monthString = "January";
        } else if (inputNumbers[0] == 2) {
            monthString = "February";
        } else if (inputNumbers[0] == 3) {
            monthString = "March";
        } else if (inputNumbers[0] == 4) {
            monthString = "April";
        } else if (inputNumbers[0] == 5) {
            monthString = "May";
        } else if (inputNumbers[0] == 6) {
            monthString = "June";
        } else if (inputNumbers[0] == 7) {
            monthString = "July";
        } else if (inputNumbers[0] == 8) {
            monthString = "August";
        } else if (inputNumbers[0] == 9) {
            monthString = "September";
        } else if (inputNumbers[0] == 10) {
            monthString = "October";
        } else if (inputNumbers[0] == 11) {
            monthString = "November";
        } else if (inputNumbers[0] == 12) {
            monthString = "December";
        }

        //Logic for testing valid inputs
        try {
            if (inputNumbers[0] > 12 || inputNumbers[0] <= 0) { //Logic for valid range of months
                throw new MonthException();
            } else if (THIRTY_DAY_MONTHS.contains(inputNumbers[0]) && inputNumbers[1] > 30) { //Logic for valid day ranges for months that only contain 30 days
                throw new DayException();
            } else if (inputNumbers[1] > 31 || inputNumbers[1] <= 0) { //Logic for max and min # of possible days per any month
                throw new DayException();
            } else if (inputNumbers[0] == 2 && inputNumbers[1] > 29 && inputNumbers[2] % 4 == 0 && (inputNumbers[2] % 100 != 0 || inputNumbers[2] % 400 == 0)) { //Logic for # of days in February for a leap year
                throw new DayException();
            } else if (inputNumbers[2] < 1000 || inputNumbers[2] > 3000) { //Logic for valid range of years
                throw new YearException();
            }
            //Prints and formats the numeric input as a string
            System.out.println(monthString + ' ' + inputNumbers[1] + ", " + inputNumbers[2]);

        } catch (MonthException e) {
            System.out.println(e.getMessage());
            loop();
        } catch (DayException e) {
            System.out.println(e.getMessage());
            loop();
        } catch (YearException e) {
            System.out.println(e.getMessage());
            loop();
        }

    }

    //Exactly the same as the above code minus the initial prompt, only runs if the user enters any invalid input
    private void loop() {...}

    public static void main(String[] args) {
        @SuppressWarnings("UnusedAssignment") Dates d = new Dates();
    }
}

Each exception class is defined like this:

class YearException extends Exception {
    public YearException() {
        super("Invalid year! Re-enter valid year!");
    }
    public YearException(String message) {
        super(message);
    }
}

I know I don't make use of the second constructor, but I was taught it's generally a best-practice to include it.

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

Some quick pointers:

  1. Fail-fast

First, your //Logic for testing valid inputs occurs after you interpret your month value, which means it is either already unnecessary after successfully setting monthString (and the other values are valid), or you just wasted code in your long if-else (see next point) should the validation fail afterwards for the day value, for example. Also, this snippet should already suggest that you should handle user inputs elsewhere instead of the constructor (see last point):

//Exactly the same as the above code minus the initial prompt, only runs if the user enters any invalid input
private void loop() {...}
  1. Long if-else

Using a Map or Enum is greatly preferred.

  1. Unused assignment

Just mentioning this for the sake of mentioning (see final point below), but you can probably get away with just this:

public static void main(String[] args) {
    new Dates();
}
  1. Doing everything in constructor

Think of public static void main as the entry point to your Java class, so handling user inputs is usually done within this method and not in the constructor. This encourages you to either use your class as an "utility class" or a "model class". The former usually implies the class will have a private constructor and some public static methods to only perform the conversion you require, while the latter usually implies that it will have some internal fields to be set upon its construction, for example. The current approach you have is creating an instance of your class to handle inputs and also do the formatting, which is not recommended. In either ways, it will also cut down on the code duplication you experienced in the first point, since you no longer need a loop() method.

  1. try-with-resources and multi try-catch

It appears that you are already using Java 7, consider try (Scanner scanner = new Scanner(System.in)) { ... } to close your Scanner instance safely. And Java 7 lets you try-catch on multiple exceptions, assuming that is part of your assignment.

\$\endgroup\$
3
\$\begingroup\$

In addition to h.j.k.'s comments....

There is a common trick you can play when dealing with data that is referenced back to a sequential number, like months. If you create an array of Month names, and an array of days in each month, like:

private static final String[] MONTH_NAMES = {"January", "February", ".....", ...};
private static final int[] MONTH_DAYS = {31,29,31,30,31,30,31,31,30,31,30,31};

Then, when the person enters a month, you can subtract 1, and index it back in to the above array, and get rid of all the if's:

int monthIndex = inputNumbers[0] - 1;
int monthDay = inputNumbers[1];
int year = inputNumbers[2];

if (monthIndex < 0 || monthIndex >= MONTH_NAMES.length) {
    throw new IllegalArgumentException("....");
}

int maxDays MONTH_DAYS[monthIndex];
String monthString = MONTH_NAMES[monthIndex];
if (monthDay < 1 || monthDay > maxDays
    || monthIndex == 1 && monthDay == 29 && (year % 4 != 0 || year % 100 == 0)) {
    throw new ....
}

Note how there is no need for the days-in-month list/set at all?

Also, note how I rename the 'inputValues' members to names that make sense.

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