Code Review: General Architecture and Error Handling

[Pablo Aguirre]

I'm replicating a question I posted on stackexchange so here's the link if you want to check out some opinions. Code review is in my opinion the single most valuable way to improve my code so any feedback is appreciated.

----
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);
            }
        }
    }
}

[Junilu Lacar]

feel free to tear it apart

Ok...

A program should tell a story. This program doesn't tell its story very well. It starts out being very vague about what it's doing by creating a FileProcessor object and then calling that object's copyFile() method. Nothing else in that code gives the reader any kind of context on what they should expect, except the error handling boilerplate around the call to copyFile(). What file is being copied? Where is it getting copied to? The code is very mysterious-looking.

When you get to the copyFile() method, you realize that there's really more to it than what the name suggests. It's doing more that just "copy a file". The JavaDocs explain enough but we'll hold off on talking about JavaDocs until we define a better organization of the code.

The copyFile() method does too many things. First, it does user input. That should be separate. The name FileProcessor is also vague whereas the JavaDocs are very specific. If the JavaDocs fully explain the reason for this class' existence, then maybe you can find a name that better reflects that purpose.

Honestly, if all I needed to do was copy one file to another then merge it with a third, I'd just do this with static methods.

This would be the pseudocode:

main code:
   get filename to copy from
   get filename to copy to
   get filename to append

   copyFile(source, dest)
   append(dest, appendFile)

copyFile(from, to):
   fromFile = openFile(from)
   toFile = createFile(to)
   toFile.write(fromFile.contents())
   close all files

append(main, other)
   mainFile = open(main)
   appendFile = open(other)
   mainFile.append(appendFile.contents())
   close all files


The semantics of the appendFile() method are also important. Your appendFile() method has source and output. That's not a very good story either. Does source identify the content that you're appending or the content that you're appending to? What is the output? If I take that name at face value, it would seem like it's supposed to identify the result of the appendFile operation. In that case, we have three things in play: the thing we're appending to (Thing1), the thing we're appending (Thing2), and the result of appending Thing2 to Thing1, Thing3. But there are only two parameters. Which one are we missing? This is confusing.

If I'm appending one thing to another, I would take it to mean that the "one thing" is what is being appended to "another" thing. That is, if I append an index to a book, then the index becomes part of the book. I won't get another book, I'll just get the same book, only now with an index.

I would expect the appendFile() method to operate in a similar manner. So that's why I have a main parameter and an other parameter and the appendFile simply modifies main by appending other to it.

[Junilu Lacar]

Pablo Aguirre wrote:I'm replicating a question I posted on stackexchange so here's the link if you want to check out some opinions.

Thanks for informing people that you've posted the same question elsewhere. You missed adding the link though so here's the one I found: https://codereview.stackexchange.com/questions/243005/architecture-and-error-handling-in-simple-java-program

[Pablo Aguirre]

Junilu Lacar wrote:

Pablo Aguirre wrote:I'm replicating a question I posted on stackexchange so here's the link if you want to check out some opinions.

Thanks for informing people that you've posted the same question elsewhere. You missed adding the link though so here's the one I found: https://codereview.stackexchange.com/questions/243005/architecture-and-error-handling-in-simple-java-program

Thank you for taking the time to reply! Yeah, I forgot to add the link, sorry. I'll rethink the code with your reply in mind. I had another comprehensive reply on stackexchange that suggests a couple of the same things. I'll probably come back soon with more questions about your reply :-D

[Pablo Aguirre]

Junilu Lacar wrote:

feel free to tear it apart

Ok...
A program should tell a story. This program doesn't tell its story very well. It starts out being very vague about what it's doing by creating a FileProcessor object and then calling that object's copyFile() method. Nothing else in that code gives the reader any kind of context on what they should expect, except the error handling boilerplate around the call to copyFile(). What file is being copied? Where is it getting copied to? The code is very mysterious-looking.

Ok, starting from here. What would be your suggestion about how to achieve this? My first modification was to add info in a javadoc block.

/**
 * This is an exercise from CodeGym, Lesson 18 - Introduction to Streams
 *
 * This program reads 3 filenames from the console
 * It copies the contents of the second file to the first file
 * It subsequently appends the contents of the third to the first file
 *
 * @author pablosouza
 *
 */

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();
        }
    }
}

[Pablo Aguirre]

Junilu Lacar wrote:The name FileProcessor is also vague whereas the JavaDocs are very specific. If the JavaDocs fully explain the reason for this class' existence, then maybe you can find a name that better reflects that purpose.

I decided to use another class mainly for practice purposes. In this case what would you suggest for the name of the class? Something more specific? FileEditor, FileCopy, FileCopyAndAppend ?

Thanks!!

[Junilu Lacar]

Pablo Aguirre wrote:My first modification was to add info in a javadoc block.

Comments are often code smells -- they indicate that the code itself could be more expressive. If you have to add a comment to explain what the code is doing, then the comment is smelly. The code should express its intent. Comments often become obsolete. Code will never be obsolete, so make the code tell its story.

[Campbell Ritchie]

That is also the wrong format for a documentation comment. The first sentence should be a brief description (=summary) of the class.