Change HashSet to a set of strings found in a file

This function changes a HashSet in the root of the class called 'commands' to a set of strings found in a file at "plugins/UC/Commands.txt" (it's what was requested by the people using the plugin). If the file doesn't exist, it copies a default file found in the zip file called "Commands.txt".

This is a weird request, and may be too much to ask for, but I realized my code is way over complicated. If you would be so kind as to show me how best to rewrite this, I would really appreciate it. When I originally wrote this, I just let Eclipse write it itself (with all the try/catch blocks being told to me), and I haven't done much to it, so it's in disarray.

public void setComs () {
Set<String> temp = new HashSet<String>();
File file = null;
InputStream dflt = null;
FileOutputStream newFile = null;
try {
fileReader = new FileReader("plugins" + File.separator + "UC" + File.separator + "Commands.txt");
try {
String line = null;
while ((line = bufferedReader.readLine()) != null) {
}
} catch (IOException e) {
System.out.println("[UC] couldn't read Commands.txt.  Setting to default");
temp.clear();
String line = null;
try {
while ((line = bufferedReader.readLine()) != null) {
}
} catch (IOException e1) {
System.out.println("[UC] Cannot read default commands file.  This is a critical error.  " +
"Message user LRFLEW on bukkit.org if you see this");
}
} finally {
try {
} catch (IOException e) {}
}
} catch (FileNotFoundException e) {
System.out.println("[UC] couldn't find Commands.txt.  Creating a new file with default settings");
file = new File("plugins" + File.separator + "UC");
try {
file.mkdir();
file = new File("plugins" + File.separator + "UC" + File.separator + "Commands.txt");
file.createNewFile();
newFile = new FileOutputStream(file);
byte[] buffer = new byte[dflt.available()];
for (int i = 0; i != -1; i = dflt.read(buffer)) {
newFile.write(buffer, 0, i);
}
temp.clear();
String line = null;
while ((line = bufferedReader.readLine()) != null) {
}
dflt.close();
} catch (IOException e1) {
System.out.println("[UC] cannot make new file.  " +
"Either I don't have enough permissions to see the file or you need to change the permissions of the folder.  " +
String line = null;
try {
while ((line = bufferedReader.readLine()) != null) {
}
} catch (IOException e2) {
System.out.println("[UC] Cannot read default file.  This is a critical error.  " +
"Message user LRFLEW on bukkit.org if you see this");
}
}

}
this.commands = temp;
}

• In regard to "Community's" comment added to the question, The FileReader's constructor can throw a FileNotFoundException and is not contained within an inner exception. – Brian Reichle Mar 26 '11 at 21:33
• What version of Java do you use? The later ones are much cleaner for file access using NIO. – Athas Apr 18 '11 at 22:38

I have not coded in Java for some time, so I encourage you to use your own discretion while reading this. It should at-least give some general direction though.

I noticed that you read lines into the hash set in several places, so I started by extracting it to a separate method.

void loadIntoHashSet(HashSet<string> set, Reader in)
{
String line = null;

{
}

}


I then noticed that you load the default values from the resource stream in two different locations and so extracted it into another method.

void loadDefaultsIntoHashSet(HashSet<string> set)
{

try
{
}
catch (IOException e1)
{
System.out.println("[UC] Cannot read default commands file.  This is a critical error.  " +
"Message user LRFLEW on bukkit.org if you see this");
}

}


I find your use of the for construct in reading the resource stream to be a little unclear and so rewrote it as:

int read;

{
}


Brian has some great suggestions so far, especially extracting reused functionality. When cleaning up a large method such as this, my first step is usually refactoring it up into smaller methods. I find short methods easier to conceptualize, clean up, and reuse (when possible).

I highly recommend the book Clean Code: A Handbook of Agile Software Craftsmanship for great coverage of writing clean, manageable code from the start. It also provides many tips on cleaning up existing code. While its examples are in Java, it applies to most every language.

Agreeing to the above (Brian, David, Anton), my impression is, that there are 4 attemps to fill the set from different locations, which play together cascadingly. But it isn't visible if you look at the code. I think there should be a main method:

fillSet () {
if (! fillfrom (x) || fillfrom (y))
fillfrom (z) || fillfrom (a);
}


Somehow like that. Instead, sometimes the exception is ignored, sometimes it is important (FileNotFound), sometimes the stream is closed immediately, sometimes in a finally. Maybe this is justified, but putting it all together seems not too easy.

However, a handicap is, that there is this premature declaration at one place:

File file = null;
InputStream dflt = null;
FileOutputStream newFile = null;


which can be easily delayed, and declared at different stages again, making the code more independent from its location. You don't save anything with a single declaration instead of 2 or 4 declarations.

Example with Paths and Files API from JDK 7 (but maybe the algorithm can be optimised):

public void setComs() {
Path directory = Paths.get("plugins" + File.separator + "UC");
Path command = directory.resolve("Commands.txt");

List<String> lines;

Path defaultSettings = Paths.get(this.getClass().getClassLoader().getResource("Commands.txt").getPath());

if (!Files.exists(command)) {
System.out.println("[UC] couldn't find Commands.txt.  Creating a new file with default settings");

try {
if (!Files.exists(directory)) {
Files.createDirectory(directory);
}

Path file = Files.createFile(command);

lines = Files.readAllLines(defaultSettings, Charset.defaultCharset());

} catch (IOException e1) {
System.out.println("[UC] cannot make new file.  " +
"Either I don't have enough permissions to see the file or you need to change the permissions of the folder.  " +

}
} else {
try {
lines = Files.readAllLines(command, Charset.defaultCharset());
} catch (IOException e) {
System.out.println("[UC] couldn't read Commands.txt.  Setting to default");

}
}

this.commands = lines;
}

private List<String> readDefaultSettings(Path defaultSettings) {
List<String> lines;
try {
lines = Files.readAllLines(defaultSettings, Charset.defaultCharset());
} catch (IOException e1) {
System.out.println("[UC] Cannot read default commands file.  This is a critical error.  " +
"Message user LRFLEW on bukkit.org if you see this");
lines = Collections.emptyList();
}
return lines;
}


Have a look at commons-io lib from Jakarta Apache. 2 classes below look extremely useful for you.

org.apache.commons.io.IOUtils
org.apache.commons.io.FileUtils


And I agree with the suggestions above. Make it more fine-grained. Idea is to have one method for one goal.

One possible option, depending on the exact details of your use case, is to use

Properties p = new Properties();
Set<String> s = Collections.checkedSet(Collections.newSetFromMap(p), String.class);


to store the data in question, rather than converting an existing Set.

The Properties class is an implementation of the Map interface which includes methods for reading from and writing its contents to a file.

Don't write what you don't need to.