CODEWITHSUNDEEP
javamultithreading
TheFriesel
TheFriesel

Reputation: 111

Calling methods from within an anonymous inner "runnable" class. Is it bad practice?

First off, let me say, I'm not the experienced Java programmer in the world to say the least.

I started using the code below quite often, when it comes to time-critical actions like internet access. This way, I can run any method in an exclusive thread. Sometimes, I even call other methods from within the first called method. I assume, they all run in the same thread. Correct me, if I'm wrong.

pingItem.addActionListener(event -> //listener of a JButton
{
    new Thread( new Runnable(){
        @Override
            public void run(){
            /*
             * some code...
             */
            showOnlineState(); //=> private static void showOnlineState()
        }
    }).start();
});

This really works well and its very simple to use., but I'm a little concerned if a thread gets "stuck" or something. Since its an anonymous class, I have no idea, how to check it with stuff like ".isAlive" or how to stop it.

Should I avoid coding like this? Is it "bad practice"?

Upvotes: 2

Views: 249

Answers (3)

Joop Eggen
Joop Eggen

Reputation: 109593

There is indeed reason to do something like that, but in this case reconsider, as it is wasteful using threads in this way.

You are using swing, and a button click should be handled fast, and no long processing should be done on the event handling thread.

For that exists SwingUtilities.invokeLater(Runnable) or SwingWorker. Look into that. The event handling thread is also where repainting is done.

Then most threads will not be necessary. Still. As physical threads are limited, a thread pool might be useful, ExecutorService. Look in examples with FutureTask too.

Basically there are no problems with threads when something throws an exception or such. But there are pitfalls in system behavior, deadlock, resources: threads that start threads. A simple architecture helps building solid software. You could make Action childs extending AbstractAction for reusability in menues and buttons.

Elaboration

The actionPerformed part can be put into an Action (interface). JButton has a constructor as such. One best derives from AbstractAction. This is a pure architectural separation, instead of adding an anonymous ActionListener. It has no influence on system behavior, but now you can do setEnabled on the Action instead of the JButton, and add the Action in the menu.

It has the same actionPerformed event handling. You can reuse the same action in a JMenuItem too. Swing uses these Actions intensively.

private Action openAction = new AbstractAction() {
    {
        putValue(..., ...); // Here you can set icon, button text,
        putValue(..., ...); // shortcuts and such.
        putValue(..., ...);
    }

    @Override
    public void actionPerformed(ActionEvent event) {
        EventQueue.invokeLater(() -> {
            ...
        });
    }
}

JButton openButton = new JButton(openAction);
JMenuItem openMenuItem = new JMenuItem(openAction);

... openAction.setEnabled(false);

For short but not instant tasks, but also for popping up a file chooser or such use in the actionPerformed:

SwingUtilities.invokeLater(() -> {
    ...
});

For longer actions, work in the background, one can use a SwingWorker, for which better examples exist elsewhere. It implements RunnableFuture, and that Future is what you are asked for:

  • get the results in a waiting manner
  • get with timeout
  • cancel
  • isCancelled
  • isDone

Upvotes: 0

Solomon Slow
Solomon Slow

Reputation: 27190

I'm a little concerned if a thread gets "stuck" or something. Since its an anonymous class, I have no idea, how to check it with stuff like ".isAlive" or how to stop it.

If you want to do things to the thread (e.g., check its state), then you have to keep a reference to it's controlling Thread object somewhere.

Thread myBackgroundThread = new Thread( new Runnable(){
    @Override
        public void run(){
        doSomethingUseful(...);
    }
});
myBackgroundThread.start();

...

if (myBackgroundThread.getState() == Thread.State.TERMINATED) {
   doSomethingAboutIt(...);
}

Calling methods from within an anonymous inner “runnable” class. Is it bad practice?

No. If you need more than a few lines of code to accomplish the work for which you created the thread, then it would be bad practice to not break it up into methods.

Upvotes: 0

duffymo
duffymo

Reputation: 308948

I would say the design could be improved:

  1. Prefer classes like Executor in the new concurrency package to JDK 1.0 Thread.
  2. The action can be altered easily if you pass in the Runnable. Using an inner class requires you to edit and redeploy the code to change it.
  3. Realize that the thread is under the control of the operating system when you set it off. You won't be able to stop it. You can set a timeout.

Upvotes: 1

Related Questions