Copy a FileInputStream to a file, either as text or as a string of bytes

Question

I have written a small program that reads input from a text file and then writes this output to a text file. The method contains a switch which decides whether to output to the file as text or as a literal string of the bytes. How could I refactor this code to only include a single if statement block instead of two to switch between these two modes? I am also interested if there is a more optimal way of writing this code overall.

I have tried to create just one switch at the beginning of the code which would change the output files from output-text-txt to output-bytes.txt (which are located in the resources folder) as well as changing the writing method (whether it writes the integer or string literals). I encounter difficulty when I need to write the bytes because the method asks for either an integer or a string and I am not sure how I would change these writing methods with just one if statement at the beginning of the code (due to it asking for int c as a parameter)

public class ByteStreamTest {

    private static File input;
    private static File output;

    public static void main(String args[]) throws Exception {
        new ByteStreamTest().write(true);
        new ByteStreamTest().write(false);     
    }

    public ByteStreamTest() {}

    public void write(boolean asText) {

        input = new File(getClass().getClassLoader().getResource("input.txt").getPath());

        if (asText) {
            output = new File(getClass().getClassLoader().getResource("output-text.txt").getPath());
        } else {
            output = new File(getClass().getClassLoader().getResource("output-bytes.txt").getPath());
        }

        FileInputStream in = null;
        FileWriter fw = null;

        try {

            in = new FileInputStream(input);
            fw = new FileWriter(output);

            int c;

            while ((c = in.read()) != -1) {
                if (asText) {
                    fw.write(c);                
                } else {
                     fw.write(String.valueOf(c) + System.getProperty("line.separator"));
                }
            }

            in.close();
            fw.close();

        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

This isn't really a question that has to do with the functionality of the code, I am simply interested in refactoring the code to include as little if statements as possibly optimizing it further.

Answer 1

Please note I am not an expert in Java, I do know something about object oriented programming.

Java is an object oriented programming language. To write good object oriented programs it's best to keep 5 programming principles in mind. Together these 5 principles are called SOLID programming. The 5 programming principles are

• The Single Responsibility Principle
• The Open-closed principle
• The Liskov substitution principle
• The Interface segregation principle
• The Dependency inversion principle

The single responsibility principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

This program violates the single responsibility principle in 2 ways, first the function main() is included in the class and second the write() function is too complex (does too much) and should be simplified.

The function write() might be better named copy(). It should do only one thing, which is copy the input file to the output file. There could be 2 copy() functions, one for text and one for strings. The decision on which way to write to the output file should be outside the copy functions.

Opening and closing the files should be outside the write function. The program should only open the input file once and possibly reset it to the beginning the second time.

The ByteStreamTest constructor could open the input file. There could be a Boolean member of the ByteStreamTest class that is either public and gets modified by main() or private with accessor functions.

Answer 2

Opening this in an IDE already shows a bunch of problems to fix.

• input and output can be local variables, in fact this is all rather procedural, so it makes sense that the containing class is just to contain the main method anyway.
• The empty constructor is pointless.
• The args parameter is written weirdly, it should be String[] args.
• throws Exception doesn't do anything.
• The catch-all block for Exception doesn't even catch all exceptions as the input and output files are opened outside of it.
• Initialisation to null is the same as not writing it explicitly.
• String.valueOf(c) doesn't do much, c itself is just fine for c + "some string" already.
• getResource can return null - here it's probably to ignore that.

And after some more manual inspection:

• Don't call close, use a try-with block.
• Perhaps only get the line separator once, not in a loop (although that's probably optimised away).
• The while loop is very inefficient, but that's probably not a concern here, so I'm not sure what to recommend. Maybe that the string concatenation can be avoided by going for two method calls instead. You'd have to profile it regardless if that makes any measurable difference at all.
• boolean arguments are easy to get wrong (that's why some IDEs show the name of argument at call sites. That can be avoided by using a builder pattern, aka new ...().asText(true).write() or something along those lines.

public class ByteStreamTest {
    public static void main(String[] args) throws IOException {
        new ByteStreamTest().write(true);
        new ByteStreamTest().write(false);
    }

    private File open(String filename) {
        return new File(getClass().getClassLoader().getResource(filename).getPath());
    }

    private void write(boolean asText) throws IOException {
        File input = open("input.txt");
        File output = open(asText ? "output-text.txt" : "output-bytes.txt");

        try (FileInputStream in = new FileInputStream(input);
             FileWriter fw = new FileWriter(output)) {
            String separator = System.getProperty("line.separator");

            int c;
            while ((c = in.read()) != -1) {
                fw.write(c);
                if (!asText) {
                    fw.write(separator);
                }
            }
        }
    }
}

---

Anyway, as an exercise how to avoid ifs it can always be rewritten in terms of two classes where one class does the true branch, the other the false branch. That also means that suddenly there's a big increase in code to read for nothing much of a benefit.

For a nicer to use API, I'd imagine something like this would make way more sense though:

public class ByteStreamTest {
    public static void main(String[] args) throws IOException {
        copy(open("input.txt"), open("output-text.txt"), true)
        copy(open("input.txt"), open("output-bytes.txt"), false)
    }

    private static void copy(File input, File output, boolean asText) throws IOException {...}
    private static File open(String filename) {...}
}

It's all procedural after all. Secondly as was already said, copy makes more sense and open can use ByteStreamTest.class.getClassLoader()... without having to actually create an object of the class.

(Why is it writing to the locations from the class loader? That wouldn't work at all if this was running from a JAR, wouldn't it?)

Answer 3

Yes, you can get rid of the if. I'd leverage a Function mapToOutput, with value either x -> x (no change) or x -> String.valueOf(x) + System.getProperty("line.separator") (your other logic).

Keep in mind that it may not actually be desirable to do this, though - it adds complexity, while only saving you a little repetition. I've done it below, and also added some comments on the rest of your code:

public class ByteStreamTest {

    private static File input;
    private static File output;
    private static Function mapToOutput;

    public static void main(String args[]) throws Exception {
        new ByteStreamTest().write(true);
        new ByteStreamTest().write(false);     
    }

    public ByteStreamTest() {}

    public void write(boolean asText) {

        input = new File(getClass().getClassLoader().getResource("input.txt").getPath());

        if (asText) {
            output = new File(getClass().getClassLoader().getResource("output-text.txt").getPath());
            mapToOutput = x -> x; // could replace with Function.identity()
        } else {
            output = new File(getClass().getClassLoader().getResource("output-bytes.txt").getPath());
            mapToOutput = x -> String.valueOf(x) + System.getProperty("line.separator");
        }

The block above is effectively setting the parameters for the block below:

        FileInputStream in = null;
        FileWriter fw = null;

These could go in a try-with-resources, e.g. try (FileInputStream in = new FileInputStream(input); FileWriter fw = new FileWriter(output)) { ...

        try {

            in = new FileInputStream(input);
            fw = new FileWriter(output);

I'm a big fan of either fr and fw (in this case you're not using a FileReader, so the point is moot), or in and out, but not the two mixed together :)

            int c;

Why c? It's not a great name.

            while ((c = in.read()) != -1) {
                fw.write(mapToOutput.apply(c));
            }

            in.close();
            fw.close();

        } catch (Exception e) {
            e.printStackTrace();
        }

It would be good to have a finally block to close your resources, or else create them in a try-with-resources. This try-catch does not guarantee your resources will be closed.

    }
}