Check robustness of function to add an interval to a date

Question

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?

Answer 1

 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.