Java GUI code with Swing Timer

Question

I have the following questions for the below code:

1. Is there a simpler implementation of an on-screen counter?
2. I made CountTimer inner class since it's tightly coupled with the GUI part anyway. What would be the best way to uncouple them?

package count_timer;

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;

public class CountTimerGUI implements ActionListener {

    private JFrame frame;
    private JPanel panel;

    private JLabel timeLabel = new JLabel();

    private JButton startBtn = new JButton("Start");
    private JButton pauseBtn = new JButton("Pause");
    private JButton resumeBtn = new JButton("Resume");
    private JButton stopBtn = new JButton("Stop");
    private JButton resetBtn = new JButton("Reset");

    private JButton greenBtn = new JButton("Green");
    private JButton redBtn = new JButton("Red");

    private CountTimer cntd;


    public CountTimerGUI() {
        setTimerText("         ");
        GUI();
    }

    private void GUI() {
        frame = new JFrame();
        panel = new JPanel();

        panel.setLayout(new BorderLayout());
        timeLabel.setBorder(BorderFactory.createRaisedBevelBorder());
        panel.add(timeLabel, BorderLayout.NORTH);

        startBtn.addActionListener(this);
        pauseBtn.addActionListener(this);
        resumeBtn.addActionListener(this);
        stopBtn.addActionListener(this);
        resetBtn.addActionListener(this);
        greenBtn.addActionListener(this);
        redBtn.addActionListener(this);

        JPanel cmdPanel = new JPanel();
        cmdPanel.setLayout(new GridLayout());

        cmdPanel.add(startBtn);
        cmdPanel.add(pauseBtn);
        cmdPanel.add(resumeBtn);
        cmdPanel.add(stopBtn);
        cmdPanel.add(resetBtn);

        panel.add(cmdPanel, BorderLayout.SOUTH);

        JPanel clrPanel = new JPanel();
        clrPanel.setLayout(new GridLayout(0,1));

        clrPanel.add(greenBtn);
        clrPanel.add(redBtn);

        panel.add(clrPanel, BorderLayout.EAST);

        frame.setContentPane(panel);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
        frame.pack();

        cntd = new CountTimer();

    }

    private void setTimerText(String sTime) {
        timeLabel.setText(sTime);
    }


    private void setTimerColor(Color sColor) {
        timeLabel.setForeground(sColor);
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        // TODO Auto-generated method stub

        JButton btn = (JButton) e.getSource();

        if (btn.equals(greenBtn))        { setTimerColor(Color.GREEN.darker()); } 
        else if (btn.equals(redBtn))     { setTimerColor(Color.RED); }
        else if (btn.equals(startBtn))   { cntd.start(); }
        else if (btn.equals(pauseBtn))   { cntd.pause(); }
        else if (btn.equals(resumeBtn))  { cntd.resume(); }
        else if (btn.equals(stopBtn))    { cntd.stop(); }
        else if (btn.equals(resetBtn))   { cntd.reset(); }
    }


    public static void main(String[] args) {

        java.awt.EventQueue.invokeLater(new Runnable() {
             public void run() {
               new CountTimerGUI();
             }
          });

    }

    private class CountTimer implements ActionListener {

        private static final int ONE_SECOND = 1000;
        private int count = 0;
        private boolean isTimerActive = false;
        private Timer tmr = new Timer(ONE_SECOND, this);

        public CountTimer() {
            count=0;
            setTimerText(TimeFormat(count));
        }

        @Override
        public void actionPerformed(ActionEvent e) {
            if (isTimerActive) {
                count++;
                setTimerText(TimeFormat(count));
            }
        }

        public void start() {
            count = 0; 
            isTimerActive = true;
            tmr.start();
        }

        public void resume() {
            isTimerActive = true;
            tmr.restart();
        }

        public void stop() {
            tmr.stop();
        }

        public void pause() {
            isTimerActive = false;
        }

        public void reset() {
            count = 0;
            isTimerActive = true;
            tmr.restart();

        }

    }

    private String TimeFormat(int count) {

        int hours = count / 3600;
        int minutes = (count-hours*3600)/60;
        int seconds = count-minutes*60;

        return String.format("%02d", hours) + " : " + String.format("%02d", minutes) + " : " + String.format("%02d", seconds);
    }
}

Answer 1

GUI:

My preference is to have one ActionListener per button. This way you won't have to deal with checking the source before performing an action.

---

Naming the method GUI() doesn't follow naming conventions. Methods should start with a lower case letter. initGUI() would be a better name as it also describes what is happening in the method.

---

The CountTimer is not a GUI component, it should not be in the GUI initialization method.

---

CountTimer:

AS you noted, CountTimer is tightly coupled with your GUI. This means you will have to do work in order to reuse it in any way. It would be better as a stand-alone class that you just happen to use in a GUI.

I believe the following would be a good interface. It provides that same kind of control and allows the user of the counter to decide what to do when an event occurs.

class `CountTimer` {
  CountTimer(TickListener listener);
  void start();
  void stop();
  void pause();
  void resume();
  void reset();
  interface TickListener {
    tick(int count);
  }
}

---

You don't set isTimerActive to false when you stop the timer. In general, this field does not give you anything because stopping the timer will stop events from being fired.

TimeFormat:

Again, this function does not follow naming conventions.

---

This should be code that is coupled with the GUI as it deals with the presentation of a number you are tracking, not the actual number value.

---

You can pass multiple arguments to String.format(), so you don't need to call it three times. Just create one format sting that accepts all the values you want.

General:

Star imports can pollute your namespace and make it hard to track down where a class is coming from.

Answer 2

+1 to @unholysampler and some other notes:

1. I'd avoid abbreviations like redBtn and cntd. They make the code harder to read and could undermine autocomplete. For example, if you type redBu autocomplete won't find anything. It's often annoying. (See also: nikie's answer on Using single characters for variable names in loops/exceptions)

2. // TODO Auto-generated method stub
   

   This comment only noise. Remove it. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

3. I think this kind of formatting is really hard to maintain:

   if (btn.equals(greenBtn))        { setTimerColor(Color.GREEN.darker()); } 
   else if (btn.equals(redBtn))     { setTimerColor(Color.RED); }
   else if (btn.equals(startBtn))   { cntd.start(); }
   else if (btn.equals(pauseBtn))   { cntd.pause(); }
   else if (btn.equals(resumeBtn))  { cntd.resume(); }
   else if (btn.equals(stopBtn))    { cntd.stop(); }
   else if (btn.equals(resetBtn))   { cntd.reset(); }
   

   If you have a new variable with a longer name you have to modify seven other lines too to keep it nice. It also looks badly on revison control diff and could cause unnecessary merge conflicts.

   From Code Complete, 2nd Edition by Steve McConnell, p758:

   Do not align right sides of assignment statements

   [...]

   With the benefit of 10 years’ hindsight, I have found that, while this indentation style might look attractive, it becomes a headache to maintain the alignment of the equals signs as variable names change and code is run through tools that substitute tabs for spaces and spaces for tabs. It is also hard to maintain as lines are moved among different parts of the program that have different levels of indentation.