2
\$\begingroup\$

I have made this simple program that computes the day the week for the years 1700 - 2399. I am looking for suggestions/ improvement advice. I am not well versed with OOP and would like suggestions on making this program more compliant with its principles. (I do know that this can be done after importing packages but wanted to do it manually for practice).

based on: http://blog.artofmemory.com/how-to-calculate-the-day-of-the-week-4203.html

import java.util.Scanner;

public class DayFinderLaunch {

    public static void main(String[] args) {

        DayFinderLaunch ob = new DayFinderLaunch();
        ob.accepter();

    }

    public void accepter() {

        Scanner sc = new Scanner(System.in);
        System.out.println("Enter the day, the month and year of concern in that order, all in numbers. \nNote: Year must be within 18th and 24th Century.\n");
        int d = sc.nextInt();
        int m = sc.nextInt() - 1;
        int y = sc.nextInt();

        DayFinder ob1 = new DayFinder();
        ob1.validCheck(d, m, y);
        sc.close();
    }
}

public class DayFinder {

    DayFinderLaunch ob2 = new DayFinderLaunch();

    public boolean isLeap(int y) {

        boolean leap = false;

        if (y % 4 == 0 && y % 100 != 0) {
            leap = true;
        }

        else if (y % 100 == 0 && y % 400 == 0) {
            leap = true;
        }

        return leap;
    }

    public void compute(int d, int m, int y) {

        int yc = ((y % 100) / 4 + (y % 100)) % 7;

        int[] mArr = { 0, 3, 3, 6, 1, 4, 6, 2, 5, 0, 3, 5 };

        String[] dArr = { "Sunday", "Monday", "Tuesday", "Wednesday",
            "Thursday", "Friday", "Saturday" };

        int mc = mArr[m];

        int cc;

        int[] cArr = { 4, 2, 0, 6, 4, 2, 0 };
        cc = cArr[y / 100 - 17];

        int sum = yc + mc + cc + d;

        if (isLeap(y) && m < 2) {

            sum = sum - 1;
        }

        int fcode = sum % 7;

        System.out.printf("\nThe day of the week on %d/%d/%d : %s", d, m + 1,
                y, dArr[fcode]);
    }

    public void validCheck(int d, int m, int y) {

        int[] mDays = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };

        if (isLeap(y)) {
            mDays[1] = 29;
        }

        if (m > 11 || m < 0 || d > mDays[m] || d < 1 || y < 1700 || y > 2399) {

            System.out.println("Invalid date, please try again.\n");
            ob2.accepter();
        } 

        else {
            compute(d, m, y);
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Declaring a new DayFinderLaunch inside DayFinder is bad (and wrong): DayFinder must find the day and nothing more (another error here: DayFinderLaunch calls DayFinder that calls DayFinderLaunch that calls DayFinder (and so on...)

compute is the main method of DayFinder, I'd rename it to findDay, getDayOfWeek or similar. You're calling it from a validation method, wrong!

Moreover, this method MUST NOT print to the console the result. A better approch is return the name of the day so the developer can choose how to manage it (Console log? File log? Message Box?)

The date validation should be done inside getDayOfWeek(). If validation fails, an exception should be throw: DayFinder MUST NOT ask for user input again, it's not its job!

...
final String dayOfWeek = getDayOfWeek(2017, 5, 30);

System.out.println("Day: " + dayOfWeek);
...
fileWriter.writeLine("Day: " + dayOfWeek);   
...

public String getDayOfWeek(final int year, final int month, final int day) {
    if (!isDateValid(year, month, day)) {
        throw new InvalidDateException(String.format("Date %d/%d/%d is not valid", year, month, day));
    }

    String dayOfWeek;

    ...

    return dayOfWeek;
}
  • I don't understand what mArr, cArr, yc, mc etc. are, you should use significative names.

  • mArr, cArr and mDays could be extracted from methods and declared as constants in DayFinder

  • dArr could become a Day enum.

Example:

public enum Day {
    SUNDAY("Sunday")
    MONDAY("Monday"),
    ...
    ...
    SATURDAY("Saturday");

    private final String name;

    private Day(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }
}

so you can retrieve the day name using the following:

String dayOfWeek = Day.values()[index].getName(); // index must be valid of course!

isLeap should be moved in a DateUtils class:

public final class DateUtils {
    public static boolean isLeap(int year) {
        return ((y % 4 == 0) && (y % 100 != 0)) 
            || ((y % 100 == 0) && (y % 400 == 0));
    }
}

I didn't check if your algorithm is correct.

\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for taking the time to reply. I am currently learning so much of what you said didn't make much sense to me. But thanks I incorporated 1/3rd of your suggestions. Hope to learn more and understand the rest 2/3rd . Cheers!! \$\endgroup\$ – CodeLearner Jun 3 '17 at 15:37

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.