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.
d
as a date and addseconds
to it, except thatseconds
is rounded up to the next 1157 days? May I ask why? \$\endgroup\$