5
\$\begingroup\$

In answering a question on SO, I provided this example. The intention was to provide a thread safe service that queues requests to make directories. I figured I could definitely use some review on my use of threading, singletons and general Java skills.

package mkdir;

import java.io.File;
import java.util.concurrent.ConcurrentLinkedQueue;

public class MkDirService implements Runnable {

    private static MkDirService service;
    private static ConcurrentLinkedQueue<File> pendingDirs;


    private MkDirService() {
        pendingDirs = new ConcurrentLinkedQueue<File>();
    }

    public static MkDirService getService() {
        if (service == null) {
            service = new MkDirService();
            new Thread(service).start();
        }
        return service;

    }

    public void makeDir(File dir) {
        pendingDirs.add(dir);
    }

    @Override
    public void run() {
        while (true) {
            while (!pendingDirs.isEmpty())
            {
                File curDir = pendingDirs.poll();
                if (curDir !=null && !curDir.exists()) {
                    curDir.mkdir();
                }
            }
            try {
                Thread.sleep(100);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

    }

}
\$\endgroup\$
2
\$\begingroup\$

pendingDirs should not be static. It's one thing to maintain a static instance to implement a singleton, but that object should interact with its state using normal instance variables. And you may as well initialize it in its declaration instead of the constructor.

The service should create the thread itself instead of creating it in getService() to make it self-contained. This allows another implementation to use a thread pool for parallel directory creation and easier testing.

getService() is not thread-safe and could allow two or more instances to be created.

Use a blocking queue, so you don't have to poll and sleep. The thread will be put to sleep if the queue is empty and be awoken once a new directory is added to the queue. This will shrink your run loop to a few lines and allow you to rely on the well-tested thread-management in java.util.concurrent.

\$\endgroup\$
3
  • \$\begingroup\$ You mean the service using MkDirService should create the Thread? That makes sense to me. Thanks for great feedback. \$\endgroup\$
    – bconneen
    Mar 4 '11 at 14:25
  • \$\begingroup\$ One of the requirements that I didn't mention, is that there should never be more than 1 thread of this service. This is to prevent conflicts. The idea behind designing this service was to give a multi-threaded application a centralized way of creating directories that could avoid conflicts. In this case would letting getService() create the thread and start it, make sense? \$\endgroup\$
    – bconneen
    Mar 4 '11 at 14:35
  • \$\begingroup\$ @bconneen - I meant to have the constructor of MkDirService create and start the thread so it's entirely self-contained. In fact, it should have start() and stop() methods where stop() a) blocks all further makeDir() calls and b) sets a flag which run() uses to exit once the queue is drained. That might be overkill for your requirements, but that's how you would allow a graceful shutdown of the service. \$\endgroup\$ Mar 4 '11 at 19:15
2
\$\begingroup\$

Also to create even one thread it is recommended to use concurrent package: Executors.newSingleThreadExecutor().submit(new MkDirService());

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.