Architecture and error handling in simple java program

Question

I'm trying to get my basic coding principles right but there's still a long way to go. This is an exercise for CodeGym. They logic itself isn't very hard and I passed the exercise easily but I'd like my code reviewed with the following in mind:

• In terms of the general architecture, I wasn't quite sure if I needed two methods. It felt tidier to me to have a method for the first task (copy) and one for the second (append) but I couldn't figure out a way to avoid calling the second method from the first.

• Should read the inputs from the console in the main method and pass the result to the class? Or it totally depends on the context of the application I'm working for?

• About error handling, I got completely lost. After doing some research I just decided to let the caller handle the error but I'm sure there's some stuff missing. I wasn't sure for example if I should have two try blocks on the first method but it also depended on the answer to the first point (architecture), so I gave up.

• It's my first time using Javadocs, so I'm not sure what I wrote is enough

• I know "Solution" is not the ideal name for the class, but I used the class created by CodeGym.

Please feel free to tear it apart :)

Thanks in advance!

public class Solution {

    public static void main(String[] args)  {
        FileProcessor fileProcessor = new FileProcessor();
        try {
            fileProcessor.copyFile();
        } catch (IOException e) {
            System.out.println("General I/O exception: " + e.getMessage());
            e.printStackTrace();
        }
    }

import java.io.*;

/**
 *  This class reads 3 filenames from console,
 *  copies content from the second file to the first and subsequently
 *  appends content from third file to first file.
 */

public class FileProcessor {
    // Study comment: Solution using "var" didn't work in this IntelliJ
    /**
     * Reads inputs, copies second file to first and calls method to append file
     * @throws IOException
     */
    public void copyFile() throws IOException {
        String outputFile;
        String sourceToCopy;
        String sourceToAppend;

         try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {
             System.out.println("Enter a filename for the output:");
             outputFile = reader.readLine();
             System.out.println("Enter the path of the file to be copied");
             sourceToCopy = reader.readLine();
             System.out.println("Enter the path of the file to be appended");
             sourceToAppend = reader.readLine();
         }

        try (FileOutputStream out = new FileOutputStream(outputFile);
             FileInputStream copy = new FileInputStream(sourceToCopy)) {
            byte[] buffer = new byte[1024];
            while (copy.available() > 0) {
                int count = copy.read(buffer);
                out.write(buffer, 0, count);
            }
        }

        appendFile(sourceToAppend, outputFile);
    }

    /**
     *
     * @param source file to append
     * @param output final result file
     * @throws IOException
     */
    public void appendFile(String source, String output) throws IOException {
        try (FileOutputStream appended = new FileOutputStream(output, true);
             FileInputStream toAppend = new FileInputStream(source)
        ) {
            byte[] buffer = new byte[1024];
            while (toAppend.available() > 0) {
                int count = toAppend.read(buffer);
                appended.write(buffer, 0, count);
            }
        }
    }
}

Answer 1

General architecture

A useful architecture guideline is:

• By introducing classes and methods, I create the high-level language that I want to use for expressing my solution.

So, It's a good idea to start from the most abstract top-level, and later implement the methods needed at that top-level code. So I'd like to rephrase your program (top-level) as follows:

• Read the output filename: String outputFile = readWithPrompt(reader, "Enter a filename for the output:");
• Read the first input filename: String sourceToCopy = readWithPrompt(reader, "Enter the path of the file to be copied");
• Read the second input filename: String sourceToAppend = readWithPrompt(reader, "Enter the path of the file to be appended");
• Combine the files: combineFiles(sourceToCopy, sourceToAppend, outputFile);

This keeps user interface and business logic apart, which I highly recommend!

Reading the three file names is three times the same, so I used a common phrase (= calling the same method, just with different prompting strings).

The main business of your program is to combine two input files into one output file, so I made that explicit at the top-level.

I like your distinct append() method. It's useful on its own, not only in your combine-two-files setting. You could even implement the whole thing by first creating an empty file, and then twice append some source file to it.

Where to read user input

Always separate user interface from doing things. Imagine that tomorrow you want to combine all files from one directory with a common footer (e.g. your company's legal stuff). Then you no longer want to ask the user for every single file, but you'll still be combining pairs of input files into an output. You can then easily re-use a clean business method, but not one mixing user I/O and business.

Error handling

I like your exception handling in the main method.

The single most important guideline is: A method should throw an exception if it couldn't do its job.

So, if during the files copying something went wrong, you ended up with a missing or corrupt result. The caller of combineFiles() can normally assume that after that call, the output file has the correct content. If not, he needs to know about that, and that's done by throwing an exception (or not catching an exception that came from some inner calls).

Exception handling is a big topic with diverging opinions between the professionals, too much to go into more details here. As long as you follow the guideline, you can't be going wrong.

Javadoc

Your Jacadocs are mostly fine. I wish most real-world programs were documented like that. Sigh!

Reads inputs, copies second file to first and calls method to append file

Here's one quirk: you say "calls method", and that's breaking encapsulation. Javadocs should describe a method as a black box, how it behaves to the outside world. For the Javadoc, it doesn't matter if the method does its job all by itself or by calling hundreds of other methods. Only describe things that have an effect visible to your caller.

When describing the append() method, you thoughts were probably withing your overall task, so the text shows parts of what the append() method is meant for ("final result file") in the whole program instead of just describing what it does (append one file to the existing contents of another file).

Class names, packages

Nothing to complain about your class names.

"Solution" is not the most inventive, but a valid name for such a compact program.

"FileProcessor" is a perfect name for a class like yours.

But when you start to do serious stuff, you should get into the habit of organizing your classes into packages with unique names, with low risk of colliding with packages from external libraries, typically by using something like a reversed domain name or identity e.g. com.facebook.username, something that's unlikely to be used by someone else.

Finally, I'd like to warn about wildcard import statements like your import java.io.*;. This makes all class names from the java.io package visible in your code. At a first glance, that's fine, you can abbreviate java.io.FileOutputStream to just FileOutputStream in your code.

But maybe, in some future Java version, a class named FileProcessor gets included into the java.io package. Then the confusion starts: does FileProcessor mean your class or the one from java.io?

While exactly that is well-defined (it will be your class as long as you're in the same package), wildcard imports can cause real trouble in related, similar situations. Every decent, modern IDE has the ability to manage individual imports for you, so replacing the wildcard import with the necessary individual imports should be "one click" and then avoids the risk I described here.

Indentation

The indentation of your code looks mostly fine, but a few lines are out of sync. So probably, you did at least some of it manually. In your IDE, look for automated code indentation or formatting. No need to waste time hitting the space key multiple times, and as a result of the automated feature, your code always looks tidy.

Answer 2

I noticed in your method copyFile, you are including input of filenames from stdin:

public void copyFile() throws IOException {
    String outputFile;
    String sourceToCopy;
    String sourceToAppend;
    try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {
        System.out.println("Enter a filename for the output:");
        outputFile = reader.readLine();
        System.out.println("Enter the path of the file to be copied");
        sourceToCopy = reader.readLine();
        System.out.println("Enter the path of the file to be appended");
        sourceToAppend = reader.readLine();
    }
    //other method
}

It is correct, but in this way if you are taking filenames from other source different from stdin like another file you cannot more use this method. A possible alternative is passing filenames as parameters and put the reading block from stdin outside the method:

public class NewSolution {
    public static void main(String[] args) throws IOException {
        String outputFile;
        String sourceToCopy;
        String sourceToAppend;

         try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {
             System.out.println("Enter a filename for the output:");
             outputFile = reader.readLine();
             System.out.println("Enter the path of the file to be copied");
             sourceToCopy = reader.readLine();
             System.out.println("Enter the path of the file to be appended");
             sourceToAppend = reader.readLine();
         }

         FileProcessor fileProcessor = new FileProcessor();
         fileProcessor.copyFile(sourceToCopy, sourceToAppend, outputFile);
    }
}

Because in your method copyFile you are not doing operations of data contained in the file but a simple copy, you could write your method using Files like below:

public static void copyFile(String sourceToCopy, String sourceToAppend, String outputFile) throws IOException {

        Files.copy(Paths.get(sourceToCopy), Paths.get(outputFile), StandardCopyOption.REPLACE_EXISTING);
        Files.copy(Paths.get(sourceToAppend), new FileOutputStream(outputFile, true));
}

Using this new version of method your class Solution can be rewritten in this way:

public class NewSolution {
    public static void main(String[] args) throws IOException {
        String outputFile;
        String sourceToCopy;
        String sourceToAppend;

         try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {
             System.out.println("Enter a filename for the output:");
             outputFile = reader.readLine();
             System.out.println("Enter the path of the file to be copied");
             sourceToCopy = reader.readLine();
             System.out.println("Enter the path of the file to be appended");
             sourceToAppend = reader.readLine();
         }
         FileProcessor.copyFile(sourceToCopy, sourceToAppend, outputFile);
    }
}

Below the new version of class FileProcessor:

public class FileProcessor {

    public static void copyFile(String sourceToCopy, String sourceToAppend, String outputFile) throws IOException {

        Files.copy(Paths.get(sourceToCopy), Paths.get(outputFile), StandardCopyOption.REPLACE_EXISTING);
        Files.copy(Paths.get(sourceToAppend), new FileOutputStream(outputFile, true));
    }
}