Writing clean code for isWithinBillingPeriod() method

[A Misa]

Hello!

I managed to write a Java method which does what is supposed to, but which i find too large and a bit confusing. I would need some help cleaning the code, maybe splitting it in two methods.

My method is called pretty straightforward: isWithinBillingPeriod(int BILLDAY, String TEMP_OPTOUT_DATE). The two parameters are the bill day (beginning of billing period, first day) and a string variable representing the date i want to find out if it's within the billing period or not. For the moment, for testing, it is a void method, but i intend making it boolean.
I'll try not to post all the code here, since is too much to read, but i'll explain by pieces.

Skipping, the first phase, where i get the current date, current day, current month, current year, and parse TEMP_OPTOUT_DATE.
Then, for calculating the billing period, there's an if statement:

if(currentDay > BILLDAY)
           {
              gc.set(GregorianCalendar.YEAR, currentYear);
              gc.set(GregorianCalendar.MONTH, currentMonth);
              gc.set(GregorianCalendar.DATE, BILLDAY);
              startDate = gc.getTime();
              start = formatter.format(startDate);        
              System.out.println("de la "+start);
              
              gc.set(GregorianCalendar.YEAR, currentYear);
              gc.set(GregorianCalendar.MONTH, currentMonth+1);
              gc.set(GregorianCalendar.DATE, BILLDAY+1);
              endDate = gc.getTime();
              end = formatter.format(endDate);        
              System.out.println("pana la "+end);
           }   
          else
           {
              gc.set(GregorianCalendar.YEAR, currentYear);
              gc.set(GregorianCalendar.MONTH, currentMonth-1);
              gc.set(GregorianCalendar.DATE, BILLDAY);
              startDate = gc.getTime();
              System.out.println(startDate);
              start = formatter.format(startDate);        
              System.out.println("de la "+start);
              
              gc.set(GregorianCalendar.YEAR, currentYear);
              gc.set(GregorianCalendar.MONTH, currentMonth);
              gc.set(GregorianCalendar.DATE, BILLDAY-1);
              endDate = gc.getTime();
              end = formatter.format(endDate);        
              System.out.println("pana la "+end); 
           }

Here, i calculate the start date and end date of the billing period, based on current day and bill day. After this, there's another if statement for finding out if TEMP_OPTOUT_DATE is within billing period or not:

  int i = searchByDate.compareTo(startDate);
       int j = searchByDate.compareTo(endDate);
       if ((i>0) && (j<0)) System.out.println("within the billing period"); 
       else System.out.println("not good");

All this is within a try catch block, catching the exception ParseException (because it didn't work otherwise).

Now, how could i split this method in two, one for billing period calculus, and one for verification, preferably, without taking TEMP_OPTOUT_DATE parameter in the first one? I was thinking about returning the startDate and endDate from the first method, and with the second, just verify where TEMP_OPTOUT_DATE is, but i don't know how to do that.

[Campbell Ritchie]

Give the variables real names. Does gc mean garbage collection?
Don't use CAPITAL_LETTERS unless the value is a constant.
Use the Java8 date classes or similar if at all possible.
Separate methods to create the billing date, current date and final date objects, please.
Separate method to print the dates in the format you want. What type of object is formatter?

Are you sure the method works correctly? What do you get from 31st January 2014 + 1 month.

[Jesper de Jong]

The correct way to add a month to a Calendar is to use the add() method; not by adding to the individual fields.

Calendar calendar = Calendar.getInstance();

// Add one month
calendar.add(Calendar.MONTH, 1);

[A Misa]

-formatter is a SimpleDateFormat object, used to bring my date in dd-MM-yyyy format
-i can't know what will happen on 31st January 2014 + 1 month, i am only able to fint the curretn billing period, minus or plus one month, i cannot test it for this example; i suppose the API should take care of that, isn't that the point of it?
-is it ok or not to use both Calendar and GregorianCalendar? Does that impact the results i get?
-how to return two values from a method? Like, from a function, to return the two dates, startDate and endDate, then to be able, from another method or something, to compare my temp_optout_date to see if it fits in the period or not?

[Campbell Ritchie]

Laura Miclescu wrote: . . .
-i can't know what will happen on 31st January 2014 + 1 month, i am only able to fint the curretn billing period, minus or plus one month, i cannot test it for this example; i suppose the API should take care of that, isn't that the point of it?
. . .

Good grief! You must test awkward cases like that. If you don't, you are risking serious errors going undetected. If the API is going to take care of it, what does the API say that ensures your method will work correctly?

[Campbell Ritchie]

Laura Miclescu wrote: . . .
-is it ok or not to use both Calendar and GregorianCalendar? Does that impact the results i get?
. . .

Have you read the documentation? What does it say about Calendar and GregorianCalendar?

[Campbell Ritchie]

Laura Miclescu wrote: . . .
-how to return two values from a method? . . .

You can't. You can however create a class which has both values as fields. There already is such a class in the Java8 date and time API. Stop using Calendar and GregorianCalendar.

[Winston Gutkowski]

Laura Miclescu wrote:My method is called pretty straightforward: isWithinBillingPeriod(int BILLDAY, String TEMP_OPTOUT_DATE). The two parameters are the bill day (beginning of billing period, first day) and a string variable representing the date i want to find out if it's within the billing period or not.

Well, that right there is a red flag, because StringsAreBad.

My advice: change your method to be:
isWithinBillingPeriod(int billDay, Date optoutDate) { ...
and work from there.

If need be, you could create a separate method to convert a String to a Date but, as has been previously mentioned, this is a basic function of SimpleDateFormat.

Alternatively, you could possibly use one of the v8 or Joda types; but these are likely far easier to create from a Date than a String anyway.

HIH

Winston

[A Misa]

I have to use the platform SE 7, cause it has to be compatible with another project i will integrate my code in, so i can't start using 8's API.
And i checked for the last month in the year, and the result is ok, it increments the year too.
Since i can't use se 8's functionality, i guess i'll add the extra code in the method for comparing the dates.

I was thinking, can i form an array with my two resulting values (startDate, endDate) and return this array?

[Jesper de Jong]

Laura Miclescu wrote:I was thinking, can i form an array with my two resulting values (startDate, endDate) and return this array?

Why not create a class BillingPeriod which has two member variables: startDate and endDate. Make the method return a BillingPeriod object - that is much more clear than returning an array with two dates.

You can even add methods to the BillingPeriod class, for example isInPeriod(Date date) to check if the specified date in the billing period.

[A Misa]

Good idea with the class. i'll try that.

Now, reffering to Winston Gutkowski response about passing the date initially as a string: at a certain point, in my code, i convert it to a date to compare it with whatever i want, and then again in string. I was just thinking it would be easier to pass it when i call my function, to have a predetermined pattern.

[Campbell Ritchie]

Passing a String would be much more error‑prone. It would also entail repeated parsing of the String. Pass an object which represents a date instead.

[Junilu Lacar]

Joda Time already has something: http://joda-time.sourceforge.net/apidocs/org/joda/time/ReadableInterval.html -- see the overlaps method.