HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Compute the day of the year

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thedayyearcompute

Problem

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 ==

Solution

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 == J

Code Snippets

if (wasLastDateInvalid)
    System.out.println("Invalid date. Please enter again.");
+--------------------+----------+---------------------+
|   Variable Name    |   case   |      expression     |
+--------------------+----------+---------------------+
|                    | negative | wasLastDateInvalid  |
| wasLastDateInvalid +----------+---------------------+
|                    | positive | !wasLastDateInvalid | Double Negation!
+--------------------+----------+---------------------+
|                    | negative | !wasLastDateValid   |
|  wasLastDateValid  +----------+---------------------+
|                    | positive | wasLastDateValid    |
+--------------------+----------+---------------------+
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;
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;
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;

Context

StackExchange Code Review Q#94974, answer score: 16

Revisions (0)

No revisions yet.