Counter GUI in Swing, updating the count in the background

Question

I made a simple counter GUI for practice. The user presses a button to start the counter. The button click executes a SwingWorker task that updates a JLabel with the count. The update occurs in the doInBackground method.

Is this a bad idea? Even for something as simple as this, I should still update the component from a method that runs on the event dispatch thread, such as process?

If so, why would it be a bad practice for something as simple as this?

Main.java:

package counter;

import javax.swing.SwingUtilities;

public class Main {
    public static void main(String[] args) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                CounterView view = new CounterView();
                view.activate();
            }
        });
    }
}

ProgressBar.java:

package counter;

import java.awt.BorderLayout;
import javax.swing.*;
import javax.swing.JFrame;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.SwingWorker;
import javax.swing.SwingConstants;
import java.awt.event.*;

public class CounterView {
    private JFrame frame;
    private JPanel gui;
    private JButton button;
    private JLabel count;

    public CounterView() {
        customizeFrame();
        createMainPanel();
        createCountText();
        createButton();
        addComponentsToFrame();
    }

    private void customizeFrame() {
        frame = new JFrame();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    private void createMainPanel() {
        gui = new JPanel();
        gui.setLayout(new BorderLayout());
    }

    private void createCountText() {
        count = new JLabel("0");
        count.setHorizontalAlignment(SwingConstants.CENTER);
    }

    private void createButton()  {
        button = new JButton("Start counter");
        button.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                // Create the worker
                SwingWorker<Void, Void> worker = new SwingWorker<Void, Void>() {

                    @Override
                    protected Void doInBackground() throws Exception {
                        for (int i = 0; i <= 50; i++) {
                            Thread.sleep(100);
                            System.out.println(i);
                            text.setText(Integer.toString(i));
                        }

                        return null;
                    }
                };

                // Start the worker
                worker.execute();
            }
        });
    }

    private void addComponentsToFrame() {
        gui.add(count, BorderLayout.CENTER);
        gui.add(button, BorderLayout.SOUTH);
        frame.add(gui);
        frame.pack();
    }

    public void activate() {
        frame.setVisible(true);
    }
}

Answer 1

To answer your question:

Q. Is this a bad idea?

A. No, it's fine in your case, but creating a new Thread is easier, like this:

private void createButton() {
    button = new JButton("Start counter");
    button.addActionListener(new ActionListener() {
        public void actionPerformed(ActionEvent e) {
            button.setEnabled(false); // can't click while counter is running
            Thread background = new Thread(new Runnable() {
                @Override
                public void run() {
                    for (int i = 0; i <= 50; i++) {
                        try {
                            Thread.sleep(100);
                        } catch(InterruptedException e) {
                            e.printStackTrace();
                        }
                        System.out.println(i);
                        count.setText(Integer.toString(i));
                    }
                    button.setEnabled(true); //click-able after the counter ends
                }
            });
            background.start();
        }
    });
}

---

There are some things you can fix in your code to improve it:

1. Use of throws Exception in your doInBackGround method. throwing Exception is not good to use because it is vague, and unnecessary in your case. Just throw InterruptedException instead.

   @Override
   protected Void doInBackground() throws InterruptedException {
       //...
   }
   

2. Error. This line: text.setText(Integer.toString(i)); throws an Error: "text cannot be resolved." I believe you meant to do: count.setText(Integer.toString(i));

3. activate() method. Make your life easier by cutting down a few lines: Just set the frame visible in your constructor, like this:

   public CounterView() {
       customizeFrame();
       createMainPanel();
       createCountText();
       createButton();
       addComponentsToFrame();
       frame.setVisible(true);
   }
   

   This removes the need for your activate method completely.

4. Notice that if you click the button twice, then 2 counters start? I don't think you want that. disable the button after it is clicked to avoid this. Example:

   private void createButton() {
       button = new JButton("Start counter");
       button.addActionListener(new ActionListener() {
           public void actionPerformed(ActionEvent e) {
               button.setEnabled(false); //added this line
               //...
   

---

Other than those, your code looks good. good indentation, variable names, method names, etc.

Answer 2

Nice code overall. Just a couple of nitpicks: a new worker (thread) is spawned every time the user presses the button. In order to circumvent it, have an atomic boolean that is set to true if a thread is counting. On subsequent presses just check the value of that value and spawn only if there is no such background thread ticking.

Also, you might be interested in providing a button for canceling the thread: just read a volatile variable in the working thread in each iteration; if the value is set to communicate that the thread should be canceled, just make it exit.