Simple GTK GUI for the command line program auCDtect

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 fileList = new ArrayList();
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

A few random notes:

-
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.

-
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

-
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

-

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.)

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

protected boolean finished = false;

-

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.)

-

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.)

-

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

StackExchange Code Review Q#29075, answer score: 3