What's the intention of using timer like this?

Question

I don't understand why using timer variable like this in the following codes.

Question 1: In both startTimer() and stopTimer(), there is a local variable aTimer to be used before the operation on timer, what is the intention?

Question 2: In stopTimer(), timer will be assigned null, so if timer is not null which means this timer is already created, when startTimer() is called, timer will not be created again. Is this best practice to check whether timer is running or not? By assign null to timer, PMD also reports violation on "NullAssignment"

private Timer timer;

private void startTimer() {
  if (timer == null) {
    Timer aTimer = timerFactory.createTimer(100000, null);
    aTimer.setListener(this);
    timer = aTimer;
  }
}

private void stopTimer() {
  if (timer != null) {
    Timer aTimer = timer;
    timer = null;
    aTimer.cancel();
    aTimer.setListener(null);
 }
}

public void start() {
  synchronized(..) {
     startTimer();
  }
}

public void stop() {
 synchronized(..) {
     stopTimer();
 }
}

no, it is not. "How to fix pmd violation "NullAssignment"?" is asked by me, but in wrong direction. thanks. — mailzyok, Jul 12 '13 at 08:22
The previous question leads to explain why PMD reports Nullassignment and GC collection. Pls find my real concerns in Question 1 and Question 2 of this question — mailzyok, Jul 12 '13 at 08:29
If you refactor, you should consider using a `ScheduledExecutorService` instead -- [`Timer` has an undocumented "feature" which is dangerous](http://stackoverflow.com/questions/17588167/what-should-timertask-scheduleatfixedrate-do-if-the-clock-changes/17588398#17588398), and what is more, `ScheduledExecutorService` is much easier to use — fge, Jul 12 '13 at 08:37
@fge I would not call it dangerous - it is probably by design (run a task at 3pm should not run at 2pm because the clock has changed) - but ScheduledExecutorService is more robust anyway. — assylias, Jul 12 '13 at 09:05
@assylias, could you provide more detail on which part is complex, and it will be great if you can give your solution, thanks in advance. — mailzyok, Jul 15 '13 at 01:00

score 1 · Accepted Answer · answered Jul 15 '13 at 10:02

Your code is equivalent to:

private boolean started = false;
private Timer timer;

public synchronized void start() {
    if (!started) {
        timer = timerFactory.createTimer(100000, null);
        timer.setListener(this);
        started = true;
    }
}

public synchronized void stop() {
    if (started) {
        timer.cancel();
        timer.setListener(null);
        started = false;
    }
}

which is shorter and easier to understand (IMHO). The local variable is superfluous since your code is synchronized and your timer variable can't be accessed by two threads concurrently.

And using the fact that timer is null as a flag for started/not started is fine with such a short code but as more lines get added, it will become confusing. I prefer a proper started flag which is self-explanatory.

Thanks @assylias. It is my expected answer, reasonable and helpful. — mailzyok, Jul 16 '13 at 02:19

score 0 · Answer 2 · answered Jul 12 '13 at 08:25

My assumption is the original author of this object wanted to encapsulate the Timer object so that you couldn't by mistake recreate it inbetween start() and stop().

This has no directly added value but perhaps if working with a large team of people on a frequently used object it's to prevent unwanted access to messing the information up, or something.

score 0 · Answer 3 · answered Jul 12 '13 at 08:35

The timer is synchronised to prevent thread interference and memory consistency errors. Synchrnizing the timer means:

No thread interference from other threads
Ensures Memory consistency
Ensures Atomic access to the timer, wherein external threads to the timer cannot access the intermediate state of the timer. i.e Either finish the action or have no action at all.

What's the intention of using timer like this?

3 Answers3