4
\$\begingroup\$

My program simply accepts the name of a directory from the user, scans said directory and generates an HTML file that lists the contents.

Is there any way that this can be improved?

/*
    Basically, take the path of a directory, get a list of all files in the directory
    and output that list to an html document
*/
import java.util.Scanner;
import java.util.ArrayList;
import java.util.Arrays;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.BufferedWriter;
import java.nio.file.FileSystemException;

public class HTMLFileListGenerator{
    static void writeDirectoryFilesToHTML(File directory) throws IOException{
        ArrayList<String> contents = new ArrayList<String>(Arrays.asList(directory.list()));

        File output = new File("output.html");
        try(BufferedWriter writer = new BufferedWriter(new FileWriter(output))){
            writer.write("<html>\n\t<body>\n\t\t<h1>Contents of " + directory.getCanonicalPath() + ":</h1>\n\t\t<ul>\n\t\t\t");

            for(int counter = 0; counter < contents.size(); counter++){
                System.out.println(contents.get(counter));
                writer.write("\t\t\t<li> " + contents.get(counter) + " </li>\n");
            }
            writer.write("\t\t</ul>\n\t</body>\n</html>");
        }
    }
    public static void main(String[] args){
        try(Scanner userInput = new Scanner(System.in)){
            File testInput;
            do{
                System.out.print("Please enter the name of the directory: ");
                String input = userInput.next();
                testInput = new File(input);
            }
            while(!testInput.isDirectory());
            writeDirectoryFilesToHTML(testInput);
        }
        catch(IOException io_exc){
            System.out.println("An error occurred: " + io_exc.getMessage());
        }
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Are you on Java 7/8? Also, do you need to generate for nested directories too? \$\endgroup\$
    – h.j.k.
    Commented Jan 21, 2016 at 5:47
  • 1
    \$\begingroup\$ I'm currently on 8 and no, no nested functionality is required. \$\endgroup\$
    – Sky
    Commented Jan 21, 2016 at 5:55

2 Answers 2

Reset to default
4
\$\begingroup\$

Files and Paths

Since Java 7, the recommended way of traversing a file system is to rely on the new Path class and associated Files and Paths helper classes.

Furthermore, since you are on Java 8, there is the Files.list(Path) method that lets you iterate through a given directory as a form of Stream<Path>, so that you can leverage on the stream-processing features.

For example, just to pad each entry in the given directory with <li>...</li>, the following snippet does something similar to your for-loop:

List<String> contents;
try {
    // assuming directoryPath is validated as a directory already
    contents = Files.list(directoryPath)
                    .map(v -> "<li>" + v + "</li>")
                    .collect(Collectors.toList());
} catch (IOException e) {
    // handle accordingly
}

To write contents out to a file, there's also Files.write(Path, Iterable, OpenOption):

try {
    Files.write(outputPath, contents);
} catch (IOException e) {
    // handle accordingly
}

However, since you need to write the header and 'close' the end of the HTML document, you can rely on the forEach() terminal operation of a Stream:

private static void writeDirectoryFilesToHTML(Path directoryPath, 
                                                Path outputPath) throws IOException {
    try (FileWriter target = new FileWriter(outputPath.toFile());
            BufferedWriter buffered = new BufferedWriter(target);
            PrintWriter writer = new PrintWriter(buffered)) {
        writer.write("<html>\n\t<body>\n\t\t<h1>Contents of "
                        + directoryPath.toRealPath() 
                        + ":</h1>\n\t\t<ul>\n");
        Files.list(directoryPath)
                .map(v -> "\t\t\t<li> " + v + " </li>")
                .forEach(writer::println);
        writer.write("\t\t</ul>\n\t</body>\n</html>");
    }
}

Here, writer::println is used as a method reference.

HTML output and OS-dependent newlines

You can also consider using some sort of a HTML output library so that you do not have to manually specify "<html>...<ul>...</ul>...</html>" (for example).

You are currently hard-coding your newlines to be \n, so you may want to create a helper method that writes the OS-dependent newlines for you, after every call to write().

Other observations

As you can see from above, it's recommended that your method takes in the output path as well, instead of assuming "output.html".

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Wow, thanks for providing me with this excellent and informative answer :) \$\endgroup\$
    – Sky
    Commented Jan 21, 2016 at 16:59
2
\$\begingroup\$

Acces modifiers

static void writeDirectoryFilesToHTML(File directory) throws IOException{

Is for this moment package private.
I'm thinking this could be either a public or if you don't want a visibility outside the class a private modifier.

For loops

for(int counter = 0; counter < contents.size(); counter++){

While it's perfectly correct, if you want a light improvement you can do this :

int contentSize = contents.size();
for(int counter = 0; counter < contentSize; counter++){

It looks maybe strange but you will save some time.
@Rolfl did already some testing about it in this answer.

Global variable

You have some text what never changes.
It's easier to make them constants like this :

private static final String HEADER_START = "<html>\n\t<body>\n\t\t<h1>Contents of ";
private static final String HEADER_END = "\t\t</ul>\n\t</body>\n</html>";

It's all grouped together and refactoring on this will be easier in the future.
Another advantage is when you run this method multiple times,
for each of those a new String()(other memory location) is created because they are local vars.
Now they are static global vars and are only once created.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Thank you for the good advice, especially for the last point :) \$\endgroup\$
    – Sky
    Commented Jan 21, 2016 at 6:59
  • 1
    \$\begingroup\$ I think a 1% savings that makes the code less standard is premature optimization. \$\endgroup\$
    – Eric Stein
    Commented Jan 21, 2016 at 14:05
  • 1
    \$\begingroup\$ @EricStein If you make yourself the habit of doing this, you don't have to do premature optimization, it's your first code what works already slightly faster. We help people to improve there knowledge so they can create better code from the first time. \$\endgroup\$
    – chillworld
    Commented Jan 21, 2016 at 14:32
  • 1
    \$\begingroup\$ It makes the code non-standard, adds an extra line, and consumes a tiny bit of extra memory. Taking a 1% runtime savings isn't worth it to me unless there's a demonstrated performance issue and that 1% makes a significant difference. \$\endgroup\$
    – Eric Stein
    Commented Jan 21, 2016 at 15:14
  • \$\begingroup\$ @EricStein Seems like mine standard is not the same as your standard coding, witch is fine. There are many standards, your way, mine way and even other ways. It's not wrong to know how you can improve code and certainly not wrong to code it like that. \$\endgroup\$
    – chillworld
    Commented Jan 21, 2016 at 17:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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