Simple GTK GUI for the command line program auCDtect

Question

I'm trying to write a simple GTK GUI for the command line program auCDtect. It is supposed to take .flac files, convert them to .wav temporarily, pass the to the auCDtect command line program to check they are lossless and then delete the temporary .wav file and output the results.

For around 10+ files it's rather slow (though admittedly so are the command line tools), so I was hoping somebody could review the code and let me know of any obvious improvements or mistakes I'm making.

All the code can be found here.

GUI class

//The user interface
import java.io.*;
import java.util.*;

import org.apache.commons.io.*;
import org.gnome.gdk.*;
import org.gnome.glib.*;
import org.gnome.gtk.*;

public class GTK extends Window implements DeleteEvent {

private String directoryPath = null;
private String filePath = null;
private Collection<String> fileList = new ArrayList<String>();
private auCDtect auCDtect;
private TextBuffer resultsBuffer;
private ProgressBar progressBar;
private double progress = 0;
private String output;
private String summary;
boolean outputUpdated = false;

public GTK() {

    //Set window title
    setTitle("Dan's AudioChecker");

    //Initialise user interface
    initUI(this);

    //Exit GUI cleanly if close pressed
    connect(this);

    //Set default size, position and make window visible
    setDefaultSize(800, 800);
    setPosition(WindowPosition.CENTER);
    showAll();
}

public void initUI(final GTK gtk) {

    setWindowIcon();

    //Create container to vertically stack widgets
    VBox vBox = new VBox(false, 5);

    //Set alignment and size of action buttons container
    Alignment halign = new Alignment(0, 0, 1, 0);

    //Create hoziontal box for action buttons (homogenous spacing - false, default spacing 10)
    HBox actionButtons = new HBox(false, 10);

    //Create horizontal box for view panes
    HBox viewPanes = new HBox(false, 10);

    //Create horizontal box for progress bar
    HBox progressBarBox = new HBox(false, 10);

    //create scrollable text box for file queue
    final TextBuffer queueBuffer = new TextBuffer();
    TextTag label = new TextTag();
    label.setBackground("#EAE8E3");
    queueBuffer.insert(queueBuffer.getIterStart(), "Queue \n", label);
    queueBuffer.insert(queueBuffer.getIterEnd(), "\n");
    TextView queue = new TextView(queueBuffer);
    queue.setEditable(false);
    queue.setCursorVisible(false);
    ScrolledWindow queueWindow = new ScrolledWindow();
    queueWindow.add(queue);

    //create scrollable text box for results info
    resultsBuffer = new TextBuffer();
    resultsBuffer.insert(resultsBuffer.getIterStart(), "Results \n", label);
    resultsBuffer.insert(resultsBuffer.getIterEnd(), "\n");
    TextView results = new TextView(resultsBuffer);
    results.setEditable(false);
    results.setCursorVisible(false);
    ScrolledWindow resultsWindow = new ScrolledWindow();
    resultsWindow.add(results);

    //Create buttons to user interface
    final FileChooserDialog directoryDialog = new FileChooserDialog("Directory", null, FileChooserAction.SELECT_FOLDER);
    Button directory = new Button("Directory");
    final FileChooserDialog filesDialog = new FileChooserDialog("Files", null, FileChooserAction.OPEN);
    FileFilter allLosslessMusic = new FileFilter("Lossless Music");
    allLosslessMusic.addMimeType("audio/flac");allLosslessMusic.addMimeType("audio/wav");
    Button files = new Button("File(s)");
    filesDialog.addFilter(allLosslessMusic);
    Label emptyLabel = new Label("");
    Button start = new Button("Start");

    //Create progress bar and add to hbox
    progressBar = new ProgressBar();
    progressBarBox.add(progressBar);

    //Make fileChooser dialog come up when directories is clicked
    directory.connect(new Button.Clicked(){

        @Override
        public void onClicked(Button directory) {
            directoryDialog.run();
            directoryPath = directoryDialog.getFilename();
            String[] extensions = {".flac", ".wav"};
            if(!(directoryPath == null)){
                Iterator<File> files = FileUtils.iterateFiles(new File(directoryPath), new SuffixFileFilter(extensions, IOCase.INSENSITIVE),DirectoryFileFilter.DIRECTORY);
                while(files.hasNext()){
                    File j =  files.next();
                    fileList.add(j.getPath());
                    queueBuffer.insert(queueBuffer.getIterEnd(), j+"\n");
                }
            }   
            directoryDialog.hide();
        }
    });

    //Make fileChooser dialog come up when files is clicked
    files.connect(new Button.Clicked(){

        @Override
        public void onClicked(Button files) {
            filesDialog.run();
            if (!(filesDialog.getFilename() == null)){
                filePath = filesDialog.getFilename();
                fileList.add(filePath);
                queueBuffer.insert(queueBuffer.getIterEnd(), filePath+"\n");
            }
            filesDialog.hide();
        }
    });

    //Make start button work
    start.connect(new Button.Clicked(){

        @Override
        public void onClicked(Button start){
            auCDtect = new auCDtect(fileList, gtk);
            Thread auCDtectThread = new Thread(auCDtect);
            auCDtectThread.start();

            Glib.idleAdd(new Handler(){
                 public boolean run(){

                     progress = auCDtect.getProgress();
                     output = auCDtect.getOutput();
                     summary = auCDtect.getSummary();

                     if(summary == null){
                         progressBar.setFraction(progress);

                          if((outputUpdated == true)&&(output != null)){
                                    resultsBuffer.insert(resultsBuffer.getIterEnd(), output+"\n");
                                    setOutputUpdated(false);
                                }

                         return true;
                     }
                     else{
                         progressBar.setFraction(progress);
                             if(summary != null){
                                resultsBuffer.insert(resultsBuffer.getIterEnd(), "\n"+"Results summary:"+"\n\n"+summary);
                            }

                         return false;
                     }
                 }
             });
        }
    });

    //Position buttons and add to fix
    actionButtons.packStart(directory, false, false, 0);
    actionButtons.packStart(files, false, false, 0);
    actionButtons.packStart(emptyLabel, true, true, 0);
    actionButtons.packStart(start, false, false, 0);

    //Add 
    viewPanes.add(queueWindow);
    viewPanes.add(resultsWindow);
    halign.add(actionButtons);
    vBox.packStart(halign, false, false, 10);
    vBox.packStart(viewPanes, true, true, 10);
    vBox.packStart(progressBarBox, false, false, 10);
    add(vBox);

    setBorderWidth(15);
}

//Method to exit application
public boolean onDeleteEvent(Widget widget, Event event) {
    Gtk.mainQuit();
    return false;
}

//Method to set window icon
public void setWindowIcon(){
//Set the audiochecker icon that appears in the start bar
Pixbuf icon = null;

try {
   icon = new Pixbuf("audiochecker.png");
} catch (FileNotFoundException e) {
    e.printStackTrace();
}

setIcon(icon);
}

public void setOutputUpdated(boolean outputUpdated){

    this.outputUpdated = outputUpdated;
}

public static void main(String[] args) {
    //Initialise and run GUI
    Gtk.init(args);
    new GTK();
    Gtk.main();
}
}

The audiochecker Class

//Checks the wav files using auCDtect command line
import java.io.*;
import java.util.*;

public class auCDtect implements Runnable { 

private String processingLog;
private String output;
private String summary;
private Collection<String> fileList;
private Collection<String> tempWavList = new ArrayList<String>();
private double progress = 0.0;
private GTK gtk;

auCDtect(Collection<String> fileList, GTK gtk){this.fileList = fileList; this.gtk = gtk;}

public void run () {

    List<String> command = new ArrayList<String>();
    command.add("./auCDtect");
    command.add("-d");
    command.add("-m10");

    //Add each song passed to this class to the auCDtect command
    for(String file:fileList){
        if(file.toLowerCase().contains("flac")){
            AudioConverter audioConverter = new AudioConverter();
            String tempWav = "tempWav.wav"+fileList.iterator();
            tempWavList.add(tempWav);
            try {
                audioConverter.decode(file, tempWav);
            } catch (IOException e) {
                e.printStackTrace();
            }
            file = tempWav;
        }
        command.add(file);
    }
    ProcessBuilder processBuilder = new ProcessBuilder(command);

        Process process = null;
        try {
            process = processBuilder.start();
        } catch (IOException e) {
            e.printStackTrace();
        }

        //Set up error stream thread 
        StreamGobbler errorGobbler = new StreamGobbler(process.getErrorStream(), "ERROR", this); 

        //Set up output stream thread
        StreamGobbler outputGobbler = new StreamGobbler(process.getInputStream(), "OUTPUT", this); 

        // Start error and input stream threads
        new Thread(errorGobbler).start(); 
        new Thread(outputGobbler).start(); 
  }

public void update(double progress, String processingLog, String output, String summary){

    this.processingLog = processingLog;
    this.output = output;
    this.summary = summary;
    this.progress = progress/(fileList.size());
    gtk.setOutputUpdated(true);
    try {
        Thread.sleep(100);
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
    if(this.progress == 1){
        deleteTempTracks();
    }
}

public double getProgress(){

    return progress;
}

public String getProcessingLog(){

    return processingLog;
}

public String getOutput(){

    return output;
}

public String getSummary(){

    return summary;
}

public void deleteTempTracks(){

    for(String tempFileName:tempWavList){
        File tempFile = new File(tempFileName);
        if(tempFile.exists()){
            tempFile.deleteOnExit();
        }
    }
}
}

The class to convert flac to WAV

import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;

import org.kc7bfi.jflac.PCMProcessor;
import org.kc7bfi.jflac.FLACDecoder;
import org.kc7bfi.jflac.metadata.StreamInfo;
import org.kc7bfi.jflac.util.ByteData;
import org.kc7bfi.jflac.util.WavWriter;

/**
 * Decode FLAC file to WAV file application.
 * @author kc7bfi
 */
public class AudioConverter implements PCMProcessor {
private WavWriter wav;

/**
 * Decode a FLAC file to a WAV file.
 * @param inFileName    The input FLAC file name
 * @param outFileName   The output WAV file name
 * @throws IOException  Thrown if error reading or writing files
 */
public void decode(String flacFile, String tempWav) throws IOException {

    FileInputStream inputStream = new FileInputStream(flacFile);
    FileOutputStream outputStream = new FileOutputStream(tempWav);
    wav = new WavWriter(outputStream);
    FLACDecoder decoder = new FLACDecoder(inputStream);
    decoder.addPCMProcessor(this);
    decoder.decode();
}

/**
 * Process the StreamInfo block.
 * @param info the StreamInfo block
 * @see org.kc7bfi.jflac.PCMProcessor#processStreamInfo(org.kc7bfi.jflac.metadata.StreamInfo)
 */
public void processStreamInfo(StreamInfo info) {
    try {
        wav.writeHeader(info);
    } catch (IOException e) {
        e.printStackTrace();
    }
}

/**
 * Process the decoded PCM bytes.
 * @param pcm The decoded PCM data
 * @see org.kc7bfi.jflac.PCMProcessor#processPCM(org.kc7bfi.jflac.util.ByteSpace)
 */
public void processPCM(ByteData pcm) {
    try {
        wav.writePCM(pcm);
    } catch (IOException e) {
        e.printStackTrace();
    }
}
}

The streamGobbler class to handle output

//Processes the output of the auCDtect command line program
import java.io.InputStream;
import java.util.Scanner;

public class StreamGobbler implements Runnable { 

InputStream inputStream; 
String type;
private String processingLog = null;
private String output = null;
private String summary = null;
protected boolean finished = false;
private auCDtect auCDtect;
private int progress = 0;

StreamGobbler(InputStream inputStream, String type, auCDtect auCDtect){this.inputStream = inputStream; this.type = type; this.auCDtect = auCDtect;} 

public void run(){ 

    Scanner scanner = new Scanner(inputStream);

    while (((scanner.hasNextLine())&&(type == "OUTPUT"))){

        String line = scanner.nextLine();

        if(line.contains("Processing file:")){
            processingLog = line.substring(line.indexOf("P"), (line.indexOf("]")+1));
        }
        if(line.contains("This track looks like")){
            output = line.substring(line.indexOf("This track"), (line.indexOf("%")+1));
            progress = progress + 1;
        }
        if(line.contains("These")){
            summary = line.substring(line.indexOf("These tracks"));
        }

        if((type == "OUTPUT")&&(progress > 0)){

            auCDtect.update(progress, processingLog, output, summary);
            processingLog = null;
            output = null;
            summary = null;
        }
    }

}
}

Answer 1

A few random notes:

1. If I'm right it's not thread safe.

   progress = auCDtect.getProgress();
   output = auCDtect.getOutput();
   summary = auCDtect.getSummary();
   

   I guess the calls above runs on an event dispatcher thread while the fields of auCDtect are set by StreamGobbler which is run on another thread, started by auCDtect.

   [...] synchronization has no effect unless both read and write operations are synchronized.

   From Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data.

2. Comments like these should be method names:

   // create scrollable text box for file queue
   ...
   // create scrollable text box for results info
   

   Eight level of indentation is too much, you should extract out some methods and classes to fulfill the single responsibility principle. Having small classes and methods means that you don't have to understand the whole program during maintenance, modifying just a small part of it is easier and requires less work.

   The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that.

   Source: Clean Code by Robert C. Martin, Chapter 3: Functions

3. I don't know that WavWriter and FLACDecoder close the FileInputStream and FileOutputStream instances that they get but I guess they aren't.

   public void decode(String flacFile, String tempWav) throws IOException {
       FileInputStream inputStream = new FileInputStream(flacFile);
       FileOutputStream outputStream = new FileOutputStream(tempWav);
       wav = new WavWriter(outputStream);
       FLACDecoder decoder = new FLACDecoder(inputStream);
       decoder.addPCMProcessor(this);
       decoder.decode();
   }
   

   Close them in a finally block or use try-with-resources. See Guideline 1-2: Release resources in all cases in Secure Coding Guidelines for the Java Programming Language

4. InputStream inputStream;
   String type;
   

   These fields could be private. (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

5. This field is never used, I think you could remove it:

   protected boolean finished = false;
   

6. StreamGobbler(InputStream inputStream, String type, auCDtect auCDtect) {
       this.inputStream = inputStream;
       this.type = type;
       this.auCDtect = auCDtect;
   }
   

   Check input values and throw an exception if they're not valid. If one of the parameters is null the run method will throw a NullPointerException later. When they're nulls it's a bug on the side of the caller and there is no point to continue the program with invalid state. Throwing an exception immediately helps debugging a lot since you get a stacktrace with the frames of the erroneous client, not just a NullPointerException on another thread. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

7. processingLog = line.substring(line.indexOf("P"), (line.indexOf("]") + 1));
   

   I guess there is a method for that in StringUtils with a readable name which expresses the intent of the statement. substringBetween looks very promising:

   StringUtils.substringBetween("wx[b]yz", "[", "]") = "b"
   StringUtils.substringBetween("yabczyabcz", "y", "z")   = "abc"
   

   (Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

8. public class auCDtect implements Runnable {
   

   According to the Code Conventions for the Java Programming Language class names should start with uppercase letters. The Java Language Specification, Java SE 7 Edition, 6.1 Declarations says the following:

   Class and Interface Type Names

   Names of class types should be descriptive nouns or noun phrases, not overly long, in mixed case with the first letter of each word capitalized.

   Also related: Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions