is there a better way to write this code for java? or to make it cleaner?

Question

i wrote this code as part of a project and this doesnt seem the most efficient. is there a cleaner way to write this method?

public static int numberMonth(int parseMonth, Boolean leapYear)
        {
            int month = 0;
            if (parseMonth < 1)
            { month = 0;
                if (parseMonth < 2)
                { month =+ 31;
                    if (parseMonth < 3)
                    {
                        if (leapYear)
                        {
                        month =+ 29;
                        }
                        else if(!(leapYear))
                        {
                            month=+28;
                            if (parseMonth < 4)
                            {
                                month =+ 30;
                                if (parseMonth < 5)
                                {


                                    month =+ 31;
                                if (parseMonth < 6)
                                {
                                    month =+ 31;
                                    if (parseMonth < 7)
                                    {
                                        month =+ 30;
                                        if (parseMonth < 8)
                                        {
                                            month =+ 31;
                                            if (parseMonth < 9)
                                            {
                                                month =+ 31;
                                                if (parseMonth < 10)
                                                {
                                                    month =+ 30;
                                                    if (parseMonth < 11)
                                                    {
                                                        month =+ 31;
                                                        if (parseMonth < 12)
                                                        {
                                                            month =+31;
                                                        }
                                                    }
                                                }
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
          }

anytime you have a large if ladder you can usually refactor into a dictionary or HashTable to look up values or execute behavior — eat_a_lemon, Mar 26 '11 at 16:40
Most assuredly, this can be written in a much cleaner fashion. — duffymo, Mar 26 '11 at 16:42
Are you sure this code does what you want? If (parseMonth < 1), then of course also (parseMonth < 2) ... (parseMonth < 12), so all of the inner if statements are quite pointless. — Philipp Wendler, Mar 26 '11 at 16:42
There is a big problem with the logic of your code (what if parseMonth >= 1)? — MByD, Mar 26 '11 at 16:45
As I can see from your code your are trying to deal with dates. Are you sure that you really need to reimplement your own method for that instead of using classes that come natively with Java? — greydet, Mar 26 '11 at 16:46
i want the code to add the days so if parseMonth = 4 then add month = 31+28/29 +31 — Max, Mar 26 '11 at 16:46
@juxstapose you don't need a dictionary/hashtable for conditional logic. — ukhardy, Mar 26 '11 at 16:46
Yes, it can be done better. But: What is it supposed to do?? — T.J. Crowder, Mar 26 '11 at 16:50
@T.J. Crowder - its supposed to add the days of the month together. so if parseMonth = 4 then month = 31+28+31 — Max, Mar 26 '11 at 16:54

score 1 · Answer 1 · edited May 23 '17 at 11:47

1

codaddict, I think that's almost right. I think he's trying to add days in the year, so you just need to throw your days variable into a loop:

public static int numberMonth(int parseMonth, String leapYear)
{
    if(parseMonth<1 || parseMonth>12) {
         return 0;
    }
    int[] monthArray = {31,28,31,30...};
    int days = monthArray[0];
    for (int ii = 1; ii < parseMonth; ii++) {
       days += monthArray[parseMonth];
    }
    if (leapYear.equals("leap") && parseMonth > 1) {
        days++;
    }
    return days;
}

But I think instead the OP should look at Calendar. Check a couple of the answers in this thread: Calculate days in year

edited May 23 '17 at 11:47

Community

1
1

answered Mar 26 '11 at 16:52

AndyMac

820
6
11

6

shouldn't it be 31,28,31,30... (March has 31 days). And it makes me crazy that leapYear isn't boolean... CRAZY I tells ya! – Yevgeny Simkin Mar 26 '11 at 17:01
Lol. Yeah ... fixed the month order in the example, but absolutely ... if he passed in a year and just used the Calendar class, he could get all the answers at once (including isLeapYear). I don't know why he'd want leapYear as a String. – AndyMac Mar 26 '11 at 17:13

score 1 · Accepted Answer · answered Mar 26 '11 at 18:37

Your original code has a lot of problems. It doesn't return a value (you obviously intend to return month, but the compiler doesn't know that. It won't reach any of the code you want it to. There are other issues too, and while they won't keep your code from working, they will keep anybody from understanding it. What does parseMonth mean? What does leapYear mean? Why can a variable called month contain values far greater than the number of months in a year? There aren't any comments to explain any of this.

If I were writing this function, I would write the following (based on AndyMac's code with some slight modifications):

public static int numberOfDaysBeforeMonth(int monthNumber, boolean leapYear)
{
    //if monthNumber is out of range, return -1
    if(monthNumber< 1 || monthNumber > 12)
         return -1;

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

    int numberOfDays = 0;

    //add up the days in the months preceding the month in question
    for (int month = 1; month < monthNumber; month++)
       numberOfDays += daysPerMonth[month - 1];

    //add an extra day if it was a leap year and the month is after February
    if (leapYear && monthNumber > 2)
        numberOfDays++;

    return numberOfDays;
}

public static int parseMonth(String input) { String month = input.substring(0,2); int monthNumber = Integer.parseInt(month); return monthNumber; } — Max, Mar 26 '11 at 18:43
public static Boolean leapYear(String input) { if (((parseYear(input) % 4 == 0) && parseYear(input) % 100 != 0) || (parseYear(input) % 4 == 0) && (parseYear(input) % 100 == 0) && (parseYear(input) % 400 == 0)) { if (parseDay(input) <= 29) {return true;} } else { return false; } } — Max, Mar 26 '11 at 18:43
I'm not saying that you need to tell ME what they mean, I'm saying that it needs to be clear to whoever will be reading your program (including yourself) in the future. If you went back and looked at this in six months, would you immediately understand what those variables were? — jonmorgan, Mar 26 '11 at 18:49
use the array to store cumulative totals instead of monthly totals, that way you don't even need a loop in here — Jean-Bernard Pellerin, Apr 10 '11 at 21:52

score 0 · Answer 3 · answered Mar 26 '11 at 17:56

There you go and by the way, the code just doesn't work. The impl. is based on so if parseMonth = 4 then month = 31+28+31.

The idea is the simplest you can get, don't sum anything on the runtime, do it once during the initialization. You can init days[] by hand but I hope it's clear now.

package t1;

import java.util.Calendar;
import java.util.GregorianCalendar;

public class MonthyDays {
    static final int[] days;
    static{
        int first = 1;//should be zero for decent logic
        days=new int[first+Calendar.UNDECIMBER];

        GregorianCalendar c=new GregorianCalendar();
        c.set(1999, 0, 1, 0, 0,0);//use some non leap year      
        for (int i=Calendar.JANUARY;i<Calendar.UNDECIMBER;i++){
            days[first+i] = c.get(Calendar.DAY_OF_YEAR)-1;
            c.add(Calendar.MONTH, 1);
        }
    }
    public static int numberMonth(int parseMonth, String leapYear){
        if (parseMonth<=0 || parseMonth>12)
            return 0;//should be IndexOutOfBounds; i.e. the chech should be removed;

        int day = days[parseMonth];
        if (parseMonth>2 && "leap".equals(leapYear))//avoid NPE
            day++;
        return day;
    }

    public static void main(String[] args) {
        System.out.println(numberMonth(2, "leap"));
        System.out.println(numberMonth(3, "leap"));

        System.out.println(numberMonth(1, "leap"));
        System.out.println(numberMonth(1, "leap"));

        System.out.println(numberMonth(12, ""));
        System.out.println(numberMonth(12, "leap"));

    }
}

is there a better way to write this code for java? or to make it cleaner?

3 Answers3

Linked