2
\$\begingroup\$

Background of the problem

My Class level variables:

    static int secondsInterval = 100000000;
    static long secondsSpecial=0;
    static Date dateSecondsReg; 
    static Integer intValue;

I have a function which uses the standard Java Calendar:

The function accepts a value in long(Which is seconds elapsed from a certain date till now) and a date(Start date) then based on this calculates seconds elapsed for sometime in future. The problem I was running into earlier was that the cal.add(Calendar.HOUR,intValue); would only accept int values as args and I used to add seconds earlier, now I changed it to Hours so that I dont get an Integer Overflow.

public static void secondsToNotify(long seconds, String d){
        Date dt = Convert(d); 
        System.out.println(""+dt);
        Calendar cal = Calendar.getInstance(); 
        cal.setTime(dt); 
        secondsSpecial = secondsInterval*(1+(seconds/secondsInterval));     
        System.out.println(""+(secondsSpecial));

        long hour = (secondsSpecial /60)/60; 
        System.out.println(""+hour);

        if(hour >= Integer.MAX_VALUE){

        }else{
            intValue =  (int) (long)hour; 
        }

        cal.add(Calendar.HOUR,intValue);    
        dateSecondsReg = cal.getTime(); 
        System.out.println(""+dateSecondsReg);
        System.out.println(""+secondsSpecial);
    }

The code in:

secondsSpecial = secondsInterval*(1+(seconds/secondsInterval));

is important for me to get an integer division to drop off the residue.

Is this fine or there is something more behind the scenes?

\$\endgroup\$
  • 1
    \$\begingroup\$ What's your goal? To parse d as a date and add seconds to it, except that seconds is rounded up to the next 1157 days? May I ask why? \$\endgroup\$ – 200_success Jan 8 '14 at 10:43
  • \$\begingroup\$ I want to predict seconds in multiples of 100000000 in future, for the date d. \$\endgroup\$ – Skynet Jan 8 '14 at 10:52
  • \$\begingroup\$ Why don't you use the answer you got on Stack Overflow for avoiding overflow? \$\endgroup\$ – 200_success Jan 8 '14 at 11:29
  • \$\begingroup\$ I am done with the overflow, the point of posting here was to double check if I am going wrong anywhere. The overflow is not present in the above code. \$\endgroup\$ – Skynet Jan 9 '14 at 2:56
2
\$\begingroup\$
 static long secondsSpecial=0;

Static variables that get modified? Rethink your approach.

If you have static variables, there's a good chance that either your design is wrong, or it should be an instance class. Always limit the scope of variables, methods and classes to the smallest one possible. A variable that is not used outside of the class should not be public, a function that only concerns the class it is defined in should not be public, a variable that is only used inside one function should not be declared in the class but in the function itself and so on.


public static void secondsToNotify(long seconds, String d){

What does d do? What is it for? Rename it according to what it role is, or provide thorough documentation on why it only is one letter.


System.out.println(""+dt);

Are you coming from a VB6 background? I wish I could say that the last time I saw someone using the "empty-string-concatenate-cast" was somewhere in the nineties...unfortunately I did not code back then and I work with a VB6 coder...so...

Why is that bad? It's some sort of, well, implicit type casting in the hope that it might work. That the Java compiler allows such thing is still a mystery to me.

Easier readable alternatives:

System.out.println(dt.toString()); // Turns out this does the same in a better way.
System.out.println(dt); // Calls the objects toString() method.

secondsSpecial = secondsInterval*(1+(seconds/secondsInterval)); 

By now I know that those variables do not have the ideal names they could have.


System.out.println(""+(secondsSpecial));

Same here...are you aware that System.out has an overload of println that accepts long?

System.out.println(secondsSpecial);

long hour = (secondsSpecial /60)/60; 

I here for inform you that this static method is not threadsafe...well, that felt funny.

If this static helper method is called from two different threads, it's possible that the first thread will set secondsSpecial, print something and while the first thread prints something the second will come along and also set secondsSpecial.

...
Thread1: Set secondsSpecial to 3
Thread2: Set the time of the calendar
Thread1: Print the value of secondsSpecial
Thread2: Set secondsSpecial to 25634
Thread1: Use secondsSpecial

And that's why static variables that get modified are bad. No one would expect a static helper method to be not threadsafe if not explicitly told about. Or would you think that Arrays.asList() is not threadsafe?


if(hour >= Integer.MAX_VALUE){

}else{

Silently failing is not acceptable, this would be a good moment to throw a IllegalArgumentException with a descriptive message.


intValue =  (int) (long)hour; 

hour is already a long.


System.out.println(""+dateSecondsReg);
System.out.println(""+secondsSpecial);

While we're at it, your function should not directly print to stdout. If you want to log something within a function, a Logger would be appropriate.


secondsSpecial = secondsInterval*(1+(seconds/secondsInterval));

is important for me to get an integer division to drop off the residue.

Please correct me, but isn't that what seconds / 60 does?

Java automatically performs an integer division if it is handed two integers/long. Anyway, this solution would be hacky at best, (int)Math.floor(value) would be better.


All in all, it sounds more like you want to rewrite the function completely and instead only accept a calendar instance which you then add the long seconds to. Like this (pseudo code):

function addSecondsToCalendar(Calendar cal, long seconds) {
    long hours = seconds / 60
    if hours >= Integer.MAX_VALUE throw IllegalArgumentException

    cal.add(hours)
}

Which raises the question what you're doing that you need this functionality. Short comparison:

Integer.MAX_VALUE:  2147483647
In minutes:         35791394.116667
In hours:           596523.23527778
In days:            24855.134803241
In years:           68.096259734906

So if you only use int seconds, you can already schedule notifications (that's what your function does, right?) for the next ~68 years.

If, on the other hand, you use "historic" dates as starting point, then it is understandable that you need a bigger range than ~68 years. Should be mentioned in the JavaDoc of the method, though.

\$\endgroup\$
  • \$\begingroup\$ This has been insightful, the part of static variables however was only due to the case that I somehow could not remove or forgot to remove it while posting it here. I had isolated the problem code form my main project and developed a new project just to check this function while the compiler was giving me warnings Cannot make a static reference to the non-static field secondsInterval. Also I accept that I am too bad at naming variables, where can I find a good naming convention guide? Thanks a ton :) \$\endgroup\$ – Skynet Jan 8 '14 at 10:17
  • \$\begingroup\$ If you haven't read it, I can only recommend Effective Java. It has a short but very useful section on names. Also if you only use variables in a function, always declare them in that function. Start with the smallest scope, meaning declare variables where you need them and only make them more accessible/visible if you have the need for it. \$\endgroup\$ – Bobby Jan 8 '14 at 10:22
  • \$\begingroup\$ Yes actually for keeping these variables Class level has a meaning in my code, the function in real code has two values of interest, so getters and setters and hence class level. \$\endgroup\$ – Skynet Jan 8 '14 at 10:25
  • \$\begingroup\$ Then it should be an instance-class in my opinion. \$\endgroup\$ – Bobby Jan 8 '14 at 10:26

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.