11
\$\begingroup\$

I am new to programming. I saw this Java code with a lot of if-else statements that is copied below. People say that such a code is called 'smelly code', so I wrote my own version of the code below. Can anyone guide me with any improvements to the program (related to style, programming, formatting or anything else)?

package dayOfYear;

/**
 *  
 * Two (smelly and not so smelly) ways to commute the day of the year,
 * when the day, month and year are entered by the user
 * 
 * @author TheProgrammer
 * 
 * Some code (mentioned below) taken from:
 * http://gsathish.github.io/eece210/a-Testing/
 * 
*/

import java.util.Scanner;

public class DayOfYear {
    public static final int JANUARY = 1, FEBRUARY = 2, MARCH = 3, APRIL = 4,
        MAY = 5, JUNE = 6, JULY = 7, AUGUST = 8, SEPTEMBER = 9, OCTOBER= 10,
        NOVEMBER = 11, DECEMBER = 12;

    public static final int MONTH_31 = 31, DAYS_IN_MONTH_31 = 31;
    public static final int MONTH_30 = 30, DAYS_IN_MONTH_30 = 30;
    public static final int MONTH_28 = 28, DAYS_IN_MONTH_28 = 28;

    public static final int ON = 1, OFF = 0;

    public static final int Month_length[]={31,28,31,30,31,30,31,
        31,30,31,30,31}; 

    public static void main(String[] args){

        int day, month, year, invalid_date_flag=OFF, month_type;
        Scanner in = new Scanner(System.in);
        do{ //do while loop to check the correctness of the entered date
            if(invalid_date_flag==ON)
                System.out.println("Invalid date. Please enter again.");

            invalid_date_flag=OFF;

            System.out.println("Enter month");
            month = in.nextInt();

            System.out.println("Enter day of the month");
            day = in.nextInt();

            System.out.println("Enter year");
            year = in.nextInt();

            if(month == FEBRUARY)
                month_type = MONTH_28;
            else if(month == JANUARY|| month == MARCH || month == MAY||
                    month == JULY || month == AUGUST || month == OCTOBER||
                    month == DECEMBER)
                month_type = MONTH_31;
            else
                month_type = MONTH_30;

            invalid_date_flag = 1;

        }while(month<JANUARY ||
                month>DECEMBER ||
                day<=0 ||
                (month_type == MONTH_31 && day > MONTH_31) ||
                (month_type == MONTH_30 && day > MONTH_30) ||
                (month_type == MONTH_28 && day > MONTH_28));        

        System.out.println(dayOfYear(month,day,year));
        in.close();
    }

    /*
     * Two (smelly and not so smelly) ways to commute the day of the    year.
     * @param month month of the year; requires month>0 and month<13
     * @param dayOfMonth day of the month
     * @param year year
     * @return the day of the year
     * For example. dayOfYear(2,1,1993)=32
     * */

    public static int dayOfYear(int month, int dayOfMonth, int year) {

        int dayOfMonth2=dayOfMonth;

        /*
         * Following if-else statements were taken from:
         * http://gsathish.github.io/eece210/a-Testing/
         * 
         * A better version of the same code is written below.
         */

        if (month == 2) {
            dayOfMonth += 31;
        } else if (month == 3) {
            dayOfMonth += 59;
        } else if (month == 4) {
            dayOfMonth += 90;
        } else if (month == 5) {
            dayOfMonth += 31 + 28 + 31 + 30;
        } else if (month == 6) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31;
        } else if (month == 7) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30;
        } else if (month == 8) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31;
        } else if (month == 9) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31;
        } else if (month == 10) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30;
        } else if (month == 11) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31;
        } else if (month == 12) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 30;
        }

        System.out.print("SmellyCode Answer: " + dayOfMonth +
                "\nNotSoSmellyCode Answer: ");

        /*Following is the better version (written by me) of the 'smelly code'
        above*/
        for( int month_index = FEBRUARY; month_index <= month; 
                month_index++ ) //months runs from FEBRUARY to month variable
            dayOfMonth2 += Month_length[month_index];   
        return dayOfMonth2;
    }
}
\$\endgroup\$
  • \$\begingroup\$ If you just want the day of year, you can use 'int dayOfYear = Calendar.get Instance().get(Calendar.DAY_OF_YEAR); \$\endgroup\$ – Datagrammar Jun 28 '15 at 15:26
15
\$\begingroup\$

Welcome to programming!

Please don't be frustrated by the amount of feedback that you'll get. Welcome and have fun!

Consider common practice

For example, common practice is that month is zero-based. I am not aware of a single API in which month is one-based. All APIs that I am aware of use month zero-based. In your code, month is one-based. That's surprising.

You can of course decide to deviate from common practice, for various reasons. Common practice isn't always best practice. As a newcomer it's difficult to judge when common practice is better than your approach and when your approach is better than common practice.

I recommend, become aware of the common practice and compare it with your approach, reason about the pros and cons of your approach vs. common practice.

A catch-all pro for common practice is the POLA - Principle of Least Astonishment (also know as the Rule of Least Surprise), as coined by Mike Cowlishaw in his book about the Rexx Programming Language. Ward Cunningham phrased this as "You know when you're reading good code when everything you read is pretty much what you'd expect."

Follow code style

There's a few minor things:

  • Put space between a keyword and (, like if ( instead of if(.
  • Follow the Java naming conventions. Change month_type to monthType etc..

It's a good idea to use an IDE with a good source code formatter, or a beautifier. I recommend IntelliJ IDEA.

Favor boolean with true and false over int with ON and OFF.

And use predicates for the names of variables and functions which hold these boolean values.

For example, int invalid_date_flag should instead be boolean wasLastDateInvalid. Then the code would read like this:

if (wasLastDateInvalid)
    System.out.println("Invalid date. Please enter again.");

Avoid double negation

Because double negation will make it harder for readers.

This starts with variable naming. wasLastDateInvalid and !wasLastDateValid have the same semantics. But the wasLastDateValid has an advantage over wasLastDateInvalid:

+--------------------+----------+---------------------+
|   Variable Name    |   case   |      expression     |
+--------------------+----------+---------------------+
|                    | negative | wasLastDateInvalid  |
| wasLastDateInvalid +----------+---------------------+
|                    | positive | !wasLastDateInvalid | Double Negation!
+--------------------+----------+---------------------+
|                    | negative | !wasLastDateValid   |
|  wasLastDateValid  +----------+---------------------+
|                    | positive | wasLastDateValid    |
+--------------------+----------+---------------------+

In many cases it's better to use positive names for variables because that avoids double negation.

Use stderr not stdout for error messages.

Your error messages are printed to stdout (Java: System.out). That hampers with automation. Error messages should be printed to stderr instead (Java: System.err).

Write small methods

For example, the code that determines the number of days in a month could be extracted in a method of its own. Ideally, methods with more than 5 lines are extremely rare. "Braces are an opportunity to extract." (Robert "Uncle Bob" C. Martin)

Make Variables final

Well, you don't need to write final everywhere in Java code. I do, others don't. The point is, avoid re-assigning variables. Instead, write smaller methods where the scope of a variable is so small that you never need to re-assign the variable.

Use meaningful variable names.

I found the method dayOfYear confusing in the beginning, until I found out that the variable dayOfMonth actually no longer contains the dayOfMonth but dayOfYear.

I recommend, do not reuse variables with different semantics. Instead, allocate a new variable for the new semantics.

Make the most frequent case the default / catch all.

In your code

if (month == FEBRUARY)
    month_type = MONTH_28;
else if (month == JANUARY|| month == MARCH || month == MAY||
        month == JULY || month == AUGUST || month == OCTOBER||
        month == DECEMBER)
    month_type = MONTH_31;
else
    month_type = MONTH_30;

the case that month_type = MONTH_31 is the most frequent case. Therefore, that should be the else branch. Your code would then be a bit shorter, like this:

if (month == FEBRUARY)
    month_type = MONTH_28;
else if (month == APRIL || month == JUNE ||
        month == SEPTEMBER || month == NOVEMBER)
    month_type = MONTH_30;
else
    month_type = MONTH_31;

Replace if or switch with []

if and switch statements which compare a value for equality which is easily convertible into an array index and assign or query a single value can be converted into arrays.

Assuming that JANUARY is 0, the following code

if (month == FEBRUARY)
    month_type = MONTH_28;
else if (month == JANUARY|| month == MARCH || month == MAY||
        month == JULY || month == AUGUST || month == OCTOBER||
        month == DECEMBER)
    month_type = MONTH_31;
else
    month_type = MONTH_30;

can be replaced by:

month_type = Month_length[month];

Your loop is good (but...)

Your loop is exactly how this lousy chain of if-statements should be replaced. Well done!

Just... are you sure that you're working with the right index in the array and yielding the correct result? Array indexes start with zero, and your FEBRUARY is 2, so you're actually skipping 2 array entries instead of 1. And if the month is December (12), it will throw ArrayIndexOutOfBoundsException.

That is, by the way, one of the reasons why in most APIs month is zero-based.

So, while your loop is good, it's not working, and needs modification. It's your "homework" to fix that loop. ;)

Test your code

Learn about Unit Testing. As already discussed, your loop is probably wrong. With a unit test, you would've discovered that.

In your case, you can do so-called gold-lead-characterization testing. You can create two methods dayOfYear(), the original one, and the new one, and you can check whether both methods yield the same output values for the same input values. The output of the original one is called gold, the output of your new method is called lead. You know your method works if the lead is identical to the gold.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot @Christian for your time and efforts to answer my question. I learned so much from it. I have made all the changes you suggested and hopefully my code is now up to the mark. github.com/TheAviralGarg/UnderstandingJava.git \$\endgroup\$ – fidgetyphi Jul 7 '15 at 4:23
7
\$\begingroup\$

Consider the following an addition to @Christian Hujer's excellent inputs. :)

Redundant code and variable names

public static final int MONTH_31 = 31, DAYS_IN_MONTH_31 = 31;
public static final int MONTH_30 = 30, DAYS_IN_MONTH_30 = 30;
public static final int MONTH_28 = 28, DAYS_IN_MONTH_28 = 28;

Your DAYS_IN_... variables are not used, so these are considered duplicate redundant code, as the numbers are already represented as MONTH_31/MONTH_30/MONTH_28.

public static final int Month_length[] = /* ... */;

In Java, variable names are often in camelCase, so that is usually written as monthLength.

Validation and leap years

Your validation essentially rests on this:

month<JANUARY || month>DECEMBER || day<=0 ||
    (month_type == MONTH_31 && day > MONTH_31) ||
    (month_type == MONTH_30 && day > MONTH_30) ||
    (month_type == MONTH_28 && day > MONTH_28)

This is a slightly roundabout way of doing things, because you are validating the month input and the day input being within the number of days in the month differently. You can in fact do both at once:

// assuming still using your 1-based month-length array
boolean isValidDayAndMonth = month >= JANUARY && month <= DECEMBER 
                                && day > 0 && day <= monthLength[month];

Ah, unfortunately leap year is something you did not consider in your implementation. Please remember to do this for the number of days in February!

Use method arguments effectively

public static int dayOfYear(int month, int dayOfMonth, int year) {
    // ...
}

year is not being used at all. This makes sense, as without leap-year validation, you don't need to know an exact year to solve your problem. Therefore, if for some reason you are happy to solve for only non-leap-year dates, this method's signature should be made simpler as such:

public static int dayOfYear(int month, int dayOfMonth) {
    // ...
}

Know your libraries

I understand you are getting started Java and you probably didn't have much time to dive in the wealth of Java's time-related API Specification, much less the good chronology-related third-party libraries such as Joda-Time. However, please take some time to look at how such calendar-based calculations can be done much more simply using existing libraries' APIs, when you are much more comfortable with Java and getting up to speed. :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks @h.k.j. for you input. I will be incorporating a method for checking leap years very shortly and I took your advice on using method arguments effectively. It was a good catch and it never occurred to be before. I have made many changes and hopefully my code is now up to the mark. github.com/TheAviralGarg/UnderstandingJava.git \$\endgroup\$ – fidgetyphi Jul 7 '15 at 4:25
4
\$\begingroup\$

You've one more step - instead of an array of month lengths, precalculate the day of year of the first day of each month, they you don't need the loop at all.

You ask for the year, but don't appear to use it to check for leap years.

You change the value of dayOfMonth then return it as being the day of year. These are different things and would be better served as separate variables if you want to put day of year in a variable. Though I'd just have the one liner:

public static int dayOfYear(int month, int dayOfMonth, int year) {
    return ( isLeapYear ( year ) ? MonthDayOffsetLeap : MonthDayOffset ) [ month ] + dayOfMonth;
}
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Very good advice, I like this one! And one of the things where C, due to the preprocessor, and C++, due to the preprocessor and templates, are stronger than Java. Java tries to perform pre-calculation, but it's limited to simple constant expressions. I think the simplest way to do this pre-calculation in Java is in a static initializer during class loading. If you really want to off-load the precalculation in a smart way creating the daysInYear[] from daysInMonth[] one would have to generate code. \$\endgroup\$ – Christian Hujer Jun 28 '15 at 17:56
  • 1
    \$\begingroup\$ @ChristianHujer Agree that C++ templates are strong, but the price is a damn complicated language. C preprocessor is a pain, which should be avoided as much as possible. Static initializers are powerful enough, though you may want to avoid them if the computation is really costly. Then you can use either code generation or precompute and store the values into a resource (which is much easier to locate than in C). \$\endgroup\$ – maaartinus Jun 28 '15 at 21:25
2
\$\begingroup\$

The first rule of writing date/time handling code is… don't do it! There are mind-boggling pitfalls everywhere. Other reviewers have pointed out the obvious omission of leap-year handling. (Note that 1700, 1800, 1900, and 2100 are not leap years according to the Gregorian calendar.) What's less obvious is the special case of the year 1582, in which December 31 was the 355th day of the year in many countries.

Therefore, you should use standard library functions to handle dates and times. I would write the function this way, using java.util.GregorianCalendar:

public static int dayOfYear(int month, int dayOfMonth, int year) {
    return new GregorianCalendar(year, month - 1, dayOfMonth).get(GregorianCalendar.DAY_OF_YEAR);
}

With Java 8, the answer is even more straightforward:

public static int dayOfYear(int month, int dayOfMonth, int year) {
    return LocalDate.of(year, month, dayOfMonth).getDayOfYear();
}

Prof. Gopalakrishnan's page, from which the smelly code is taken, advocates code review and unit testing. I'm glad that you are doing code review, but you aren't doing unit testing. (You can visually inspect the output, which is not the same thing as automated testing.) It is very difficult to unit-test code that uses System.out.print() as you have done.

The standard framework for unit testing in Java is JUnit. If, as a , you don't want to deal with the complexity of JUnit yet, you could use this technique to verify whether the old and new implementations behave identically:

public class DayOfYear {
    public static int dayOfYear(int month, int dayOfMonth, int year) {
        /*
         * Following if-else statements were taken from:
         * http://gsathish.github.io/eece210/a-Testing/
         */

        if (month == 2) {
            dayOfMonth += 31;
        } else if (month == 3) {
            dayOfMonth += 59;
        } else if (…) {
            …
        } else if (month == 12) {
            dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 30;
        }
        return dayOfMonth;
    }
}

public class DayOfYearImproved extends DayOfWeek {
    public static final int JANUARY = 1, FEBRUARY = 2, MARCH = 3, APRIL = 4,
        MAY = 5, JUNE = 6, JULY = 7, AUGUST = 8, SEPTEMBER = 9, OCTOBER= 10,
        NOVEMBER = 11, DECEMBER = 12;

    private static final int[] MONTH_LENGTH = {31,28,31,30,31,30,31,31,30,31,30,31};

    public static int dayOfYear(int month, int dayOfMonth, int year) {
        int d = dayOfMonth;
        for( int month_index = FEBRUARY; month_index <= month; 
                month_index++ ) //months runs from FEBRUARY to month variable
            d += MONTH_LENGTH[month_index];
        assert(d == DayOfYear.dayOfYear(month, dayOfMonth, year));
        return d;
    }
}

… and it should be immediately clear that the results don't match.


You omitted the braces from your for loop. I don't care where you put the braces (as long as it's consistent), but good quality code should always include the "optional" braces. By omitting braces, you will likely contribute to a future coding accident, and it will be all your fault.


Instead of

public static final int Month_length[] = { … };

you should write

private static final int MONTH_LENGTH[] = { … };

because

  • Code in other classes can overwrite array contents despite the fact that the array is final. The final merely guarantees that Month_length cannot be later changed to point to an entirely new array. Making it private means that only code in this class has access to the array, and of course you would be careful not to trash your own array.
  • Constants should be written in ALL_CAPS.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Why the hell would you tell him not to do it? Anything that helps you learn is outstanding keep it up. \$\endgroup\$ – 13aal Nov 14 '15 at 21:10
  • 1
    \$\begingroup\$ @LostBam Unlike other exercises, calendar code is less of a test of programming skill than of trivia, such as standard-to-summer time adjustments, how the Gregorian transition happened in each country, the time when some Pacific islands skipped a day, Russia's recent time zone redefinition, and how Shanghai adjusted its clock by a few minutes in 1927. Better to spend the effort learning to use existing libraries. \$\endgroup\$ – 200_success Nov 14 '15 at 21:30
  • \$\begingroup\$ @200_sucess well I can't say that I agree but I can see where you're coming from. \$\endgroup\$ – 13aal Nov 15 '15 at 4:07

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.