0
\$\begingroup\$

You are given the following information, but you may prefer to do some research for yourself.

  • 1 Jan 1900 was a Monday.
  • Thirty days has September, April, June and November.
  • All the rest have thirty-one, Saving February alone, Which has twenty-eight, rain or shine.
  • And on leap years, twenty-nine.
  • A leap year occurs on any year evenly divisible by 4, but not on a century unless it is divisible by 400.

How many Sundays fell on the first of the month during the twentieth century (1 Jan 1901 to 31 Dec 2000)?

public static void problem_19() {            
        int day = 4; // Lets number the days of week from 1:7 && SaturDay is the first day;;; note : 1 jan 1901 was Tue day
        int sundays = 0;
        for(int year = 1901; year <= 2000; year++) 
            for(int mon = 1; mon <= 12; mon++) {
                day += days(mon, year);
                if(day%7 == 1)
                    sundays++;
            }
        System.out.println(sundays);
    }
    public static int days(int num, int year) {
        int res = 0;
        int feb = 28;
        if(isLeapYear(year))
            feb = 29;
        switch(num) {
            case 1 : res = 31; break;
            case 2 : res = feb; break;
            case 3 : res = 31; break;
            case 4 : res = 30; break;
            case 5 : res = 31; break;
            case 6 : res = 30; break;
            case 7 : res = 31; break;
            case 8 : res = 31; break;
            case 9 : res = 30; break;
            case 10 : res = 31; break;
            case 11 : res = 30; break;
            case 12 : res = 31; break;
        }
        return res;
    }
    public static boolean isLeapYear(int year) {
        if(year%4 == 0) 
            if(year%100 == 0) {
                if(year%400 == 0)
                    return true;
            } else
                return true;
        return false;
    }
\$\endgroup\$
4
\$\begingroup\$
        int day = 4; // Lets number the days of week from 1:7 && Sunday is the first day;;; note : 1 jan 1901 was Tue day

The comment gets lost on the right edge of this narrow screen.

The position of the comment implies that it is specific to the variable day, but that's not true of the first sentence.

For working with % it would be less confusing to use 0 to 6 rather than 1 to 7.

Given that day = 4, why does the comment tell us that 1 jan 1901 was Tue day? What does day represent here? If it's 1900-12-01 then the comment should talk about that date. In general, day is not a self-explanatory name.


            for(int mon = 1; mon <= 12; mon++) {

mon for Monday? Again, not the most helpful name.


                day += days(mon, year);
                if(day%7 == 1)
                    sundays++;

Is overflow of day a risk?


    public static int days(int num, int year) {
        int res = 0;
        int feb = 28;
        if(isLeapYear(year))
            feb = 29;
        switch(num) {
            case 1 : res = 31; break;
            case 2 : res = feb; break;
            ...
            case 12 : res = 31; break;
        }
        return res;
    }

num? That's completely uninformative. I know it's a number because I can see that its type is int.

Early returns would make this shorter and more obviously correct:

        switch (month) {
            case 1 : return 31;
            case 2 : return isLeapYear(year) ? 29 : 28;
            ...

It's bad practice to have a switch without a default, even if the only thing the default does is throw new Exception("This should be unreachable code").

One option would be to group the cases according to a well-known rhyme, and pull the sanity checking out of the switch:

        if (month < 1 || month > 12) throw new IllegalArgumentException("month");

        switch (month) {
            // 30 days have September, April, June and November...
            case 9:
            case 4:
            case 6:
            case 11:
                return 30;

            // ... All the rest have 31 ...
            default:
                return 31;

            // ... except for February alone.
            case 2:
                return isLeapYear(year) ? 29 : 28;
        }

    public static boolean isLeapYear(int year) {
        if(year%4 == 0) 
            if(year%100 == 0) {
                if(year%400 == 0)
                    return true;
            } else
                return true;
        return false;
    }

Simplifying from the inside out, and putting in the necessary {} to be sure that the code does what we expect, we get:

    public static boolean isLeapYear(int year) {
        if (year % 4 == 0) {
            if (year % 100 == 0) {
                return year % 400 == 0;
            }
            return true;
        }
        return false;
    }
    public static boolean isLeapYear(int year) {
        if (year % 4 == 0) {
            return y % 100 > 0 || year % 400 == 0;
        }
        return false;
    }
    public static boolean isLeapYear(int year) {
        return year % 4 == 0 && (y % 100 > 0 || year % 400 == 0);
    }
    public static boolean isLeapYear(int year) {
        return (year % 4 == 0 && y % 100 > 0) || year % 400 == 0;
    }

Of course, given that we're only interested in years 1901 to 2000, this could be simplified further.

\$\endgroup\$
  • \$\begingroup\$ Thanks alot. this review is what i looking for. \$\endgroup\$ – Omar Ahmed Sep 13 at 11:34
0
\$\begingroup\$

Fairly simple code. On thing that caught my eye was the isLeapYear method. I think the logic would be easier to understand by keeping it in one line:

if((year % 4 == 0 && year % 100 != 0) || year % 400 == 0){
     return true;
}
\$\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.