[QUESTION] Java: Help with Optimizing Code

[stark025]

Hi. I am a student, and I have an report to do for our Computer Programming Fundamentals class.

We are required to demonstrate the use and function of constructors and the JOptionPane class.

I have implemented a simple age calculator using Java as my demo, and I intend to go through the code to show them what each line does, however I'd like to ask the forumers here for advice on optimizing my code so that it is:

a) smaller
b) simpler
c) well-documented (prefer this)
d) faster (prefer this over size)

I used three classes:

a Main class (http://pastebin.com/BBT0pkWt);

// Submitted by Ray Anthony Uy Pating for COMPROG1
package _Java_Assignment_Constructors_and_JOptionPane;
import java.util.Calendar;
import java.util.GregorianCalendar;

public class Main {
	/**
	 * @param args
	 */
	public static void main(String[] args) {
		// TODO Auto-generated method stub
		GregorianCalendar date = new GregorianCalendar();
      	GlobalVariables.year = date.get(Calendar.YEAR);
      	GlobalVariables.month = date.get(Calendar.MONTH);
      	GlobalVariables.day = date.get(Calendar.DATE);
		// Get current system date and time
        
		// Call primary getDate function to get user input
		UserInput.getYear();
		UserInput.getMonth();
		UserInput.getDay();
		ProgramOutput.displayAge(GlobalVariables.userYear, GlobalVariables.year, GlobalVariables.userMonth, GlobalVariables.month, GlobalVariables.userDay, GlobalVariables.day);
	}
}
/**
 * @author Ray Pating
 */

a UserInput class to gather user input (http://pastebin.com/94KRwgDi);

package _Java_Assignment_Constructors_and_JOptionPane;
import javax.swing.JOptionPane;

/**
 * @author Ray Pating

 */
public class UserInput {

	public static void getYear()
	{
		try {
			GlobalVariables.usrYear = (String)JOptionPane.showInputDialog(null, "Input Birth Year", "Please enter your birth year: ", JOptionPane.QUESTION_MESSAGE, null, null, "2010");
			GlobalVariables.userYear = Integer.parseInt(GlobalVariables.usrYear);
			if (GlobalVariables.userYear < (GlobalVariables.year - 150) || (GlobalVariables.userYear > GlobalVariables.year)){
				JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid year to continue.\n(YEAR_OUT_OF_BOUNDS)", "Year Invalid.", JOptionPane.ERROR_MESSAGE);
				getYear();
			}
		} catch (NumberFormatException e) {
			JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid year to continue.", "Year Invalid.", JOptionPane.ERROR_MESSAGE);
			getYear();
		}
	}
	
	// Generate modal dialog boxes for user input for Month. The _try_ statement is used to catch errors when non-integers are entered.		
	// An Object array called 'months' is used to create an array of strings for use in the dropdown box.
	// Multiple If-Else If-Else statements were used to convert the array to integers for use.
	// The lack of String support for switch and case statements is one of the flaws of Java.
	
	public static void getMonth()
	{
			Object[] months = {"January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"};
			GlobalVariables.usrMonth = (String)JOptionPane.showInputDialog(null, "Input Birth Month", "Please enter your birth month: ", JOptionPane.QUESTION_MESSAGE, null, months, "January");
			if (GlobalVariables.usrMonth.equals("January")) {GlobalVariables.userMonth = 1;}
			else if (GlobalVariables.usrMonth.equals("February")) {GlobalVariables.userMonth = 2;}
			else if (GlobalVariables.usrMonth.equals("March")) {GlobalVariables.userMonth = 3;}
			else if (GlobalVariables.usrMonth.equals("April")) {GlobalVariables.userMonth = 4;}
			else if (GlobalVariables.usrMonth.equals("May")) {GlobalVariables.userMonth = 5;}
			else if (GlobalVariables.usrMonth.equals("June")) {GlobalVariables.userMonth = 6;}
			else if (GlobalVariables.usrMonth.equals("July")) {GlobalVariables.userMonth = 7;}
			else if (GlobalVariables.usrMonth.equals("August")) {GlobalVariables.userMonth = 8;}
			else if (GlobalVariables.usrMonth.equals("September")) {GlobalVariables.userMonth = 9;}
			else if (GlobalVariables.usrMonth.equals("October")) {GlobalVariables.userMonth = 10;}
			else if (GlobalVariables.usrMonth.equals("November")) {GlobalVariables.userMonth = 11;}
			else if (GlobalVariables.usrMonth.equals("December")) {GlobalVariables.userMonth = 12;}
			else {
				JOptionPane.showMessageDialog(null, "Program has encountered a validation error. Please re-enter the correct month to continue.", "Validation Error.", JOptionPane.ERROR_MESSAGE);
				getMonth();
				 }
	}
	// Generate modal dialog boxes for user input for Day. The _try_ statement is used to catch errors when non-integers are entered.	
	// Sanity checks for valid day values for each month are also implemented
	public static void getDay() {
		try {
			GlobalVariables.usrDay = (String)JOptionPane.showInputDialog(null, "Input Day of Birth", "Please enter your day of birth: ", JOptionPane.QUESTION_MESSAGE, null, null, "1");
			GlobalVariables.userDay = Integer.parseInt(GlobalVariables.usrDay);
			// Check for nonzero Day value
			if (GlobalVariables.userDay != 0){
				// Check if the Birth Year is a correct centenary leap year or leap year. 
				// The function will return false if either the year is a centenary non-leap-year or not a leap year.
				// A leap year is a year that is divisible by both 4 or 400. 
				if (((GlobalVariables.userYear % 4) == 0) && ((GlobalVariables.userYear % 100) != 0)){
							if (GlobalVariables.userMonth == 2){
								if (GlobalVariables.userDay > 29){
									JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day to continue. (DAY_OUT_OF_BOUNDS)\n" + "Specified month has only 29 days for that year.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
									getDay();
								}
							}
				else if ((GlobalVariables.userYear % 400) == 0){
					if (GlobalVariables.userMonth == 2){
						if (GlobalVariables.userDay > 29){
							JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day to continue. (DAY_OUT_OF_BOUNDS)\n" + "Specified month has only 29 days for that year.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
							getDay();
							}
						}
					}
				}
				// Handles non-leap year Februarys and the other days of the month.
				else if (GlobalVariables.userMonth == 2){
						if (GlobalVariables.userDay > 28){
							JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day to continue. (DAY_OUT_OF_BOUNDS)\n" + "Specified month has only 28 days for that year.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
							getDay();
						}
				}
				else if ((GlobalVariables.userMonth == 4) || (GlobalVariables.userMonth == 6) || (GlobalVariables.userMonth == 9) || (GlobalVariables.userMonth == 11)) {
					if (GlobalVariables.userDay > 30){
						JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day to continue. (DAY_OUT_OF_BOUNDS)\n" + "Specified month has only 30 days for that year.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
						getDay();
						}
					}
					if (GlobalVariables.userDay > 31){
						JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day to continue. (DAY_OUT_OF_BOUNDS)\n" + "Specified month has only 31 days for that year.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
						getDay();
						}
					}
			else {
				JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day to continue. (NONZERO_VALUE_REQUIRED)\n" + "Zero is not a valid day of birth.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
				getDay();
			}
		} catch (NumberFormatException e) {
			JOptionPane.showMessageDialog(null, "Invalid input specified. Please enter a valid day of birth to continue.", "Day of Birth Invalid.", JOptionPane.ERROR_MESSAGE);
			getDay();
		}
	}
}

a ProgramOutput class to display the output of the program (http://pastebin.com/DYGLsNqG);

package _Java_Assignment_Constructors_and_JOptionPane;
import javax.swing.JOptionPane;

/**
 * @author Ray Pating
 */
public class ProgramOutput {

	public static void displayAge(int yr, int tdYr, int mth, int tdMth, int usday, int tdDay)
  	{
  		int usrYear, year, usrMonth, month, usrDay, day, outDay=0, outMonth, outYear, tempMth;
  		String pyr = new String();
  		String pmth = new String();
  		String pdy = new String();
  		int [] dayInMonth = {31,28,31,30,31,30,31,31,30,31,30,31};
  		int tempDyMth; 
  		pyr = "year";
  		pmth = "month";
  		pdy = "day";
  		usrYear = yr;
  		year = tdYr;
  		usrMonth = mth;
  		month = tdMth;
  		usrDay = usday;
  		day = tdDay;
  		outDay = day - usrDay;
  		tempMth = usrMonth;
  		outMonth = (month+1) - usrMonth;
  		outYear = year - usrYear;
  			if (usrMonth-1<month&&usrDay>day)
      			{
      				outMonth = outMonth - 1;
      				tempDyMth = dayInMonth[usrMonth-1];
      				outDay = tempDyMth+outDay;
      			}
      			if ( outDay<0 )
          			{
          			outDay = getDaysInMonth(tempMth,usrDay,day);
          			outMonth = Math.abs(outMonth);
          			outMonth=(12-outMonth)-1;
          			outYear=outYear-1;
          			}
          		if (outMonth<0)
          		{	
          			outMonth=Math.abs(outMonth); 
          			outMonth=12-outMonth;
          			outYear=outYear-1;
          		}
          		if( outYear > 1 )
          			pyr = "years";
          		if( outMonth > 1 )
          			pmth = "months";
          		if( outDay > 1 )
          			pdy = "days";
          		
          		String currentMonth = new String();
          		if ((month + 1) == 1) {currentMonth = "January";}
          		if ((month + 1) == 2) {currentMonth = "February";}
          		if ((month + 1) == 3) {currentMonth = "March";}
          		if ((month + 1) == 4) {currentMonth = "April";}
          		if ((month + 1) == 5) {currentMonth = "May";}
          		if ((month + 1) == 6) {currentMonth = "June";}
          		if ((month + 1) == 7) {currentMonth = "July";}
          		if ((month + 1) == 8) {currentMonth = "August";}
          		if ((month + 1) == 9) {currentMonth = "September";}
          		if ((month + 1) == 10) {currentMonth = "October";}
          		if ((month + 1) == 11) {currentMonth = "November";}
          		if ((month + 1) == 12) {currentMonth = "December";}
          	
          		JOptionPane.showMessageDialog(null, "Your birthday is on " + GlobalVariables.usrMonth + " " + GlobalVariables.userDay + ", " + GlobalVariables.userYear +". \n" + "As of today (" + currentMonth +" "+day+", "+year+") you are "
          			 + outYear + " " + pyr + " "
          			+ outMonth + " " + pmth + " "
          			+ outDay + " " + pdy + " old.", "Your Calculated Age.", JOptionPane.INFORMATION_MESSAGE);
          	}
          		 
          	private static int getDaysInMonth(int usrMth, int usrDy, int tdDay)
              	{
              		int usrMonth, usrDay, daysInMth,day, outDay;
              		int [] dayInMonth = {31,28,31,30,31,30,31,31,30,31,30,31};
              		usrMonth = usrMth-1;
              		usrDay = usrDy;
              		day = tdDay;
              			daysInMth = dayInMonth[usrMonth];
              			outDay = daysInMth - usrDay;
              			outDay = outDay + day;
              		return outDay;
              	}
}

and a GlobalVariables class to contain public static variables to simulate "global" variables (http://pastebin.com/DCh71xu8).

// Submitted by Ray Anthony Uy Pating for COMPROG1
// This class simply creates public static variables as methods, thus creating "pseudo"-global variables
// This is a known weakness of Java (No support for global variables)
package _Java_Assignment_Constructors_and_JOptionPane;

public class GlobalVariables {
    public static int year;
    public static int month;
    public static int day;
    public static int outYear;
    public static int outDay;
    public static int outMonth;
    public static int userYear;
    public static int userMonth;
    public static int userDay;
    public static String usrYear = new String();
    public static String usrDay = new String();
    public static String usrMonth = new String();
    public static String input = new String();
}

Please check my code, and suggest improvements.

[NormR1]

Your conversions from month of year (int) to the name of the month looks weak.
Anytime there is a bunch of repetitive if's like that look to put the data in a collection of some kind with a lookup or direct indexing.

The list of ifs should be if{} else if since there is only one match and when its made their is no need to look further. You did in the first case but not the second.
Only do the month+1 one time and use its value vs continually redoing the sum.

[stark025]

Thanks for the advice. The first post is updated with your advice. I replaced all the repetitive ifs, instead creating an Object[] array populated with the names of each month, then filling in currentmonth with the object array's elements, using the variable months as the index. It saved me 12 lines of if operations.

The new code for the ProgramOutput.java class is as below:

package _Java_Assignment_Constructors_and_JOptionPane;
import javax.swing.JOptionPane;

/**
 * @author Ray Pating
 */
public class ProgramOutput {

	public static void displayAge(int yr, int tdYr, int mth, int tdMth, int usday, int tdDay)
  	{
  		int usrYear, year, usrMonth, month, usrDay, day, outDay=0, outMonth, outYear, tempMth;
  		String pyr = new String();
  		String pmth = new String();
  		String pdy = new String();
  		int [] dayInMonth = {31,28,31,30,31,30,31,31,30,31,30,31};
  		int tempDyMth; 
  		Object[] month_names = {"January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November","December"};
  		pyr = "year";
  		pmth = "month";
  		pdy = "day";
  		usrYear = yr;
  		year = tdYr;
  		usrMonth = mth;
  		month = tdMth;
  		usrDay = usday;
  		day = tdDay;
  		outDay = day - usrDay;
  		tempMth = usrMonth;
  		outMonth = (month+1) - usrMonth;
  		outYear = year - usrYear;
  			if (usrMonth-1<month&&usrDay>day)
      			{
      				outMonth = outMonth - 1;
      				tempDyMth = dayInMonth[usrMonth-1];
      				outDay = tempDyMth+outDay;
      			}
      			if ( outDay<0 )
          			{
          			outDay = getDaysInMonth(tempMth,usrDay,day);
          			outMonth = Math.abs(outMonth);
          			outMonth=(12-outMonth)-1;
          			outYear=outYear-1;
          			}
          		if (outMonth<0)
          		{	
          			outMonth=Math.abs(outMonth); 
          			outMonth=12-outMonth;
          			outYear=outYear-1;
          		}
          		if( outYear > 1 )
          			pyr = "years";
          		if( outMonth > 1 )
          			pmth = "months";
          		if( outDay > 1 )
          			pdy = "days";
          		
          		String currentMonth = new String();
          		currentMonth = month_names[month].toString();
          	
          		JOptionPane.showMessageDialog(null, "Your birthday is on " + GlobalVariables.usrMonth + " " + GlobalVariables.userDay + ", " + GlobalVariables.userYear +". \n" + "As of today (" + currentMonth +" "+day+", "+year+") you are "
          			 + outYear + " " + pyr + " "
          			+ outMonth + " " + pmth + " "
          			+ outDay + " " + pdy + " old.", "Your Calculated Age.", JOptionPane.INFORMATION_MESSAGE);
          	}
          		 
          	private static int getDaysInMonth(int usrMth, int usrDy, int tdDay)
              	{
              		int usrMonth, usrDay, daysInMth,day, outDay;
              		int [] dayInMonth = {31,28,31,30,31,30,31,31,30,31,30,31};
              		usrMonth = usrMth-1;
              		usrDay = usrDy;
              		day = tdDay;
              			daysInMth = dayInMonth[usrMonth];
              			outDay = daysInMth - usrDay;
              			outDay = outDay + day;
              		return outDay;
              	}
}

-- Your conversions from month of year (int) to the name of the month looks weak.

Weak how? Please explain in simpler terms. I'm not that advanced of a programmer.

[NormR1]

Putting the month names in the array fixed that.

[stark025]

Thanks. Any other gems of wisdom to improve my code?

[NormR1]

if (usrMonth-1<month&&usrDay>day)

I find This is hard to read.
I prefer using () around all expressions and to have spaces around operators

outDay = day - usrDay;

What is this for? Add comment says why you are computing this value.

[stark025]

Thanks. Changes implemented.

[stark025]

For part of the code, I had to implement this to retrieve the month as an integer

Object[] months = {"January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"};
			GlobalVariables.usrMonth = (String)JOptionPane.showInputDialog(null, "Input Birth Month", "Please enter your birth month: ", JOptionPane.QUESTION_MESSAGE, null, months, "January");

My question is this: Is there any way to make

JOptionPane.showInputDialog(null, "Input Birth Month", "Please enter your birth month: ", JOptionPane.QUESTION_MESSAGE, null, months, "January");

return the index value in the object array

month[]

, instead of a string containing the contents of the array? Doing so would eliminate the if branches to a single statement.

[NormR1]

The answer would be in the API doc for that class and methods you are using.

[stark025]

Thanks. Although the answer wasn't in the JOptionPane API documentation, it was in the java.util.Array documentation, so it's all cool.

The old code was:

Object[] months = {"January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"};
			GlobalVariables.usrMonth = (String)JOptionPane.showInputDialog(null, "Input Birth Month", "Please enter your birth month: ", JOptionPane.QUESTION_MESSAGE, null, months, "January");
			if (GlobalVariables.usrMonth.equals("January")) {GlobalVariables.userMonth = 1;}
			else if (GlobalVariables.usrMonth.equals("February")) {GlobalVariables.userMonth = 2;}
			else if (GlobalVariables.usrMonth.equals("March")) {GlobalVariables.userMonth = 3;}
			else if (GlobalVariables.usrMonth.equals("April")) {GlobalVariables.userMonth = 4;}
			else if (GlobalVariables.usrMonth.equals("May")) {GlobalVariables.userMonth = 5;}
			else if (GlobalVariables.usrMonth.equals("June")) {GlobalVariables.userMonth = 6;}
			else if (GlobalVariables.usrMonth.equals("July")) {GlobalVariables.userMonth = 7;}
			else if (GlobalVariables.usrMonth.equals("August")) {GlobalVariables.userMonth = 8;}
			else if (GlobalVariables.usrMonth.equals("September")) {GlobalVariables.userMonth = 9;}
			else if (GlobalVariables.usrMonth.equals("October")) {GlobalVariables.userMonth = 10;}
			else if (GlobalVariables.usrMonth.equals("November")) {GlobalVariables.userMonth = 11;}
			else if (GlobalVariables.usrMonth.equals("December")) {GlobalVariables.userMonth = 12;}
			else {
				JOptionPane.showMessageDialog(null, "Program has encountered a validation error. Please re-enter the correct month to continue.", "Validation Error.", JOptionPane.ERROR_MESSAGE);
				getMonth();
				 }

The new code (and solution to the question) is:

public static void getMonth()
	{
			String[] months = {"January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"};
			GlobalVariables.userMonth = java.util.Arrays.asList(months).indexOf(JOptionPane.showInputDialog(null, "Input Birth Month", "Please enter your birth month: ", JOptionPane.QUESTION_MESSAGE, null, months, "January"));
			GlobalVariables.usrMonth = (months[GlobalVariables.userMonth]);
				 }

My question is this: Which of the two executes faster?

[NormR1]

Sometimes execution speed is not as important as ease of understanding and ease of maintenance. The first one is harder to maintain because of all the repeated statements that could be mis-typed when changed.