Correct control of execution of Java thread

I'm writing an application that will use the JLayer library to play an mp3 file. Since the player does not have any methods to pause the playback - play() does not return until the song is finished - I decided to put it in a thread and simply control that thread. This is possible because one can execute play(1) which plays a single frame of the song. Putting this in a loop will thus play the entire song smoothly.

What I have done is the following:

• Create a Thread class Mp3PlayerThread that can be used to play a song in a different thread.

• Create a wrapping class Mp3Player that holds a thread object and controls the volatile flags inside the Thread.

I built the object Mp3Player such that it should be thread safe. I.e., pass an instance of Mp3Player around. To do this I made all the control methods synchronized.

I am wondering if this code can be made more solid without using volatile variables? I have always learned that you should not use the volatile keyword if possible.

Mp3PlayerThread

public class Mp3PlayerThread extends Thread
{
    public  volatile boolean playing = false;
    public  volatile boolean abort   = false;
    private Player   player;
    private String   path;

    public Mp3PlayerThread(String file)
    {
        this.path = file;
    }

    public void run()
    {
        // http://stackoverflow.com/questions/7313657/should-you-synchronize-the-run-method-why-or-why-not
        try
        {
            FileInputStream     fis = new FileInputStream(this.path);
            BufferedInputStream bis = new BufferedInputStream(fis);
            this.player             = new Player(bis);

            while (!abort)
            {
                if (playing)
                    player.play(1);
            }
            Printer.debugMessage(this.getClass(), "end of file or stopped");

        } catch (FileNotFoundException | JavaLayerException e)
        {
            e.printStackTrace();
        }
    }
}

Mp3

Lets start with the instance variables.

public class Mp3PlayerThread extends Thread
{
    public  volatile boolean playing = false;
    public  volatile boolean abort   = false;
    private Player   player;
    private String   path;
    //...
}

-
You shouldn't be directly exposing playing and abort. Doing this hinders your ability to change the internal implementation of your class and remain backwards compatible.

-
Trying to line up you variable declarations will be a losing battle. It looks nice now, but when you add a new variable, you might have to completely change everything. This will make looking at diffs harder because some lines will be changed even though they are not directly involved in the new functionality. Additionally, an argument could be made that all of the variable names should be lined up instead of having player line up with boolean.

-
path is set once in the constructor and never changed. Setting it to be final could prevent you from accidentally changing it later with the expectation of it having an effect.

The URL at the beginning of the run method is not helpful. There is no context explaining why it is relevant to this piece of code. The post asks about something that this code is not doing. The general consensus of the post is "there is no reason to do that and I have never seen that done."

public void run() {
    FileInputStream     fis = new FileInputStream(this.path);
    BufferedInputStream bis = new BufferedInputStream(fis);
    this.player             = new Player(bis);
    //...
}

By writing your class this way, it is always forced to read a file from disk. What happens if you want to stream the song over a socket? Passing the input stream in to the constructor (or even the Player instance) would make your class more flexible and easier to test.

while (!abort)
{
    if (playing)
        player.play(1);
}

If playing is set to false, this thread will spin and waste cpu time.

There is no reason Mp3PlayerThread to be a subclass of Thread. Making it a simple class that implements Runnable would allow you to use it in more contexts and not incur the heavy costs of creating a thread every time an instance is instantiated. An example of the expanded context would be to create a thread pool of size one. Then you can schedule multiple songs to be played just by adding an instance to the execution pool for each new song.

public Mp3Player(String filename)
{
    song = filename;
}

public Mp3PlayerThread(String file)
{
    this.path = file;
}

Why are the variable names changing when they are all the same thing? Variable shadowing is not a valid excuse as you can use this. or prefix your member variables with an underscore.

In general, you should decide if you want to use this. or not and be consistent.

StackExchange Code Review Q#61375, answer score: 6