6
\$\begingroup\$

I am using traceview and DDMS to analyse my operation. The following is my traceview as displayed in DDMS:

enter image description here

According to the above, the LoaderClass.months() function takes about 51% of the CPU time and 798.80 System time. I have reproduced this function below:

public int months(String date) {

        SimpleDateFormat f = new SimpleDateFormat("yyyy-MM-dd");
        Date d1 = null;
        try {
            d1 = f.parse(date);
        } catch (ParseException e) {

            e.printStackTrace();
        }
        Date d2 = new Date();  
        Calendar c1 = Calendar.getInstance();
        c1.setTime(d1);
        Calendar c2 = Calendar.getInstance();
        c2.setTime(d2);
        int diff = 0;
        if (c2.after(c1)) {
            while (c2.after(c1)) {
                c1.add(Calendar.MONTH, 1);
                if (c2.after(c1)) {
                    diff++;
                }
            }
        } else if (c2.before(c1)) {
            while (c2.before(c1)) {
                c1.add(Calendar.MONTH, -1);
                if (c1.before(c2)) {
                    diff--;
                }
            }
        }
        return diff;
    }

Below is a more detailed view of the method as on DDMS:

enter image description here

\$\endgroup\$
10
\$\begingroup\$

Don't reinvent the wheel for what Joda time has already written for you:

int monthsBetween = Months.monthsBetween(new LocalDate(date1), new LocalDate()).getMonths();

From the localDate doc:

  1. You can pass a string in the constructor. (you could test that)
  2. Empty constructor is current time.
\$\endgroup\$
  • \$\begingroup\$ I highly regret that I did not use Joda Time. Now I have advanced too much in the project and reverting back is not an option. \$\endgroup\$ – Skynet Aug 8 '14 at 5:32
  • \$\begingroup\$ you don't have to revert, add the dependency and this will work, your date1 and date2 are normal java.util.Date \$\endgroup\$ – chillworld Aug 8 '14 at 5:36
  • \$\begingroup\$ I will have to make changes in each of my function - to map them to Joda time, that is time consuming. However I will for sure check it out right now. \$\endgroup\$ – Skynet Aug 8 '14 at 5:50
  • \$\begingroup\$ Can I invite you to the chat to discuss furter? \$\endgroup\$ – chillworld Aug 8 '14 at 5:52
  • 3
    \$\begingroup\$ Did it with JODA, performance optimized by at least 65 percent! Cheers mate! \$\endgroup\$ – Skynet Aug 8 '14 at 12:40
8
\$\begingroup\$

I don't like the naming style you're using. Single and double-letter identifiers are annoying.

Use meaningful names!

Isn't this:

SimpleDateFormat f = new SimpleDateFormat("yyyy-MM-dd");

More enjoyable to read like that?

SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd");

Also, it's not immediately apparent what the method does - months looks more like a variable name than a method, because it doesn't start with a verb.

A more meaningful name could perhaps be getElapsedMonths... and then the xx2 variables could be named xxPresent, and xx1 could be xxSpecified, so getElapsedMonths would calculate the number of months elapsed since a specified date.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I will definitely consider your advise, but the problem here is not with the naming that slows down the function, but the call to add does. It would be a huge help if you have any hints on how to improve that as well. \$\endgroup\$ – Skynet Aug 8 '14 at 5:49

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.