• Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Writing clean code for isWithinBillingPeriod() method

 
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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:



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:



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.
 
Marshal
Posts: 79398
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.
 
Java Cowboy
Posts: 16084
88
Android Scala IntelliJ IDE Spring Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The correct way to add a month to a Calendar is to use the add() method; not by adding to the individual fields.

 
A Misa
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
-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
Marshal
Posts: 79398
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Marshal
Posts: 79398
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Marshal
Posts: 79398
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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.
 
Bartender
Posts: 10780
71
Hibernate Eclipse IDE Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Java Cowboy
Posts: 16084
88
Android Scala IntelliJ IDE Spring Java
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

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
Ranch Hand
Posts: 47
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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
Marshal
Posts: 79398
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
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.
 
Sheriff
Posts: 17652
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Joda Time already has something: http://joda-time.sourceforge.net/apidocs/org/joda/time/ReadableInterval.html -- see the overlaps method.
 
Because those who mind don't matter and those who matter don't mind - Seuss. Tiny ad:
a bit of art, as a gift, that will fit in a stocking
https://gardener-gift.com
reply
    Bookmark Topic Watch Topic
  • New Topic