patternjavaModerate
Compute the day of the year
Viewed 0 times
thedayyearcompute
Problem
I am new to programming. I saw this Java code with a lot of
```
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 ==
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:
It's a good idea to use an IDE with a good source code formatter, or a beautifier. I recommend IntelliJ IDEA.
Favor
And use predicates for the names of variables and functions which hold these
For example,
Then the code would read like this:
Avoid double negation
Because double negation will make it harder for readers.
This starts with variable naming.
But the
In many cases it's better to use positive names for variables because that avoids double negation.
Use
Your error messages are printed to
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
Well, you don't need to write
Use meaningful variable names.
I found the method
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
the case that
Therefore, that should be the
Your code would then be a bit shorter, like this:
Replace
Assuming that
```
if (month == FEBRUARY)
month_type = MONTH_28;
else if (month == JANUARY|| month == MARCH || month == MAY||
month == J
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
(, likeif (instead ofif(.
- Follow the Java naming conventions. Change
month_typetomonthTypeetc..
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
finalWell, 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.