6
\$\begingroup\$

I've made the following Java-class just for getting some practice with the creation and reading of files.

package modul;

import static java.lang.System.*;
import java.io.*;

public class Sandbox {
    public static void main (String[] args) {

        // I found out that the project directory is the
        //  working directory by default.
        //  Is there a way to change the working directory in Java?
        //  So that all instructions become relative to that directory.
        String currentDir = System.getProperty("user.dir") + "/src/modul/"; 
        String fileName = "lorem.txt";

        try {
            PrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(currentDir + fileName)));

            // Write a few lines of dummy-text ...
            writer.println("The very first line of text.");
            writer.println("----------------------------");
            writer.println("Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor.");
            writer.println("Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim.");
            writer.println("In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo.");

            // Is it enough just closing the PrintWriter?
            //  What's about the BufferedReader and the FileReader?
            //  Are they closed automatically?
            writer.close();     

            // Now open the created file. Read and print it's 
            //  content to stdout.
            BufferedReader reader = new BufferedReader(new FileReader(currentDir + fileName));
            String line;
            int lineNumber = 0;

            while ((line = reader.readLine()) != null) {
                out.println(++lineNumber + ": " + line);
            }

            // Is it fair enough just closing the Reader?
            reader.close(); 
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            e.printStackTrace();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Although it won't serve a real use-case it's important for me to know if everything is done in a good way. Or does I have to improve the code?

Moreover: Are there ways to accomplish writing-, reading-operations more efficient? Respectively in a simpler way?

A few more questions I've still got about Java IO-operations I've added as comments within the code.

All comments, hints and recommendations of more experienced Java-devs much appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ It is considered standard on Code Review to leave the question open (i.e., no accepted answers) for at least a day so that new answerers have motivation to answer, existing answers are be improved, etc. \$\endgroup\$ – Tamoghna Chowdhury May 5 '17 at 10:50
  • \$\begingroup\$ @TamoghnaChowdhury Sorry. Will keep that in mind for the future. And thanks for the hints. \$\endgroup\$ – michael.zech May 6 '17 at 2:54
8
\$\begingroup\$

Nice first implementation.

Autoclosable / try-with-resources

Please consider the AutoClosable feature from Java 8 7 to use in a try-with-resources block. See here for more detail.

Split try blocks to be as small as possible

Try (ha :) to isolate as much code as possible in your try blocks. You can make better error handling that way.

Reuse classes

Always search the Java API for nice helper classes, in this case there is a LineNumberReader that will do what you want to do

Changing working directory

I really hate it when programs do this. You will (on exit) end up in a different locatation, unless you put effort in switching back, which makes you program more difficult.

Best thing to do it to establish a 'base' directory, and use relative paths from there. Just like you did.

Proposed solution

    try (PrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(currentDir + fileName))))
    {

        // Write a few lines of dummy-text ...
        writer.println("The very first line of text.");
        writer.println("----------------------------");
        writer.println("Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor.");
        writer.println("Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim.");
        writer.println("In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo.");

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

   try(LineNumberReader lineLeader = new LineNumberReader( new BufferedReader(new FileReader(currentDir + fileName))))
    {
        // Now open the created file. Read and print it's 
        //  content to stdout.in 
        String line;

        while ((line = lineLeader.readLine()) != null) {
            out.println(lineLeader.getLineNumber() + ": " + line);
        }
    }
    catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    } catch (Exception e) {
        e.printStackTrace();
    }
\$\endgroup\$
  • \$\begingroup\$ The only thing I miss in your answer is the piping of Exceptions. \$\endgroup\$ – slowy May 5 '17 at 9:12
  • \$\begingroup\$ You cannot 'pipe' (multi-catch) the exceptions, because the Exception will eat them all :) \$\endgroup\$ – RobAu May 5 '17 at 9:23
  • 3
    \$\begingroup\$ Ooooh, right, my bad (low coffee level). Then I'd suggest do get rid of FileNotFound- and IOException, with the reason, that the handling of those is the same as in Exception anyway. \$\endgroup\$ – slowy May 5 '17 at 9:33
  • \$\begingroup\$ Thanks for the bunch of awesome answers. Sadly I can accept only one answer. But they all were useful to me. Definitely! \$\endgroup\$ – michael.zech May 5 '17 at 10:05
  • 2
    \$\begingroup\$ I do not know java that much, but why would you (as a user) end in a different directory? Working dir is a property of a process, so the java process will not change the working directory of the user's shell. \$\endgroup\$ – Edheldil May 5 '17 at 11:54
9
\$\begingroup\$
            PrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(currentDir + fileName)));
            ...
            BufferedReader reader = new BufferedReader(new FileReader(currentDir + fileName));

Never ever ever open a file for reading or writing characters without explicitly specifying the encoding. You'll think it's all working fine and then someone will run your program on Mac OS X and send you a file encoded in MacRoman because that's the default on their platform.

Yes, I learnt that one the hard way.

Unfortunately the Java standard library tries to make things "easy" by hiding access to the encoding and ends up making it hard for anyone who wants to write portable code (supposedly Java's strong point). You can't use FileWriter, because none of its constructors have an encoding parameter. You have to create a FileOutputStream and wrap it in an OutputStreamWriter. FileReader is similarly useless.

\$\endgroup\$
  • \$\begingroup\$ +1 for the encoding. Very important. Cost me loads of time in the past as well. In this example the default encoding is used twice, so it won't be an issue. \$\endgroup\$ – RobAu May 5 '17 at 8:55
  • \$\begingroup\$ btw. Apache commons provides the very useful FileWriterWithEncoding \$\endgroup\$ – RobAu May 5 '17 at 9:00
  • \$\begingroup\$ +1, encoding can be a PITA when you go multiplatform - but there's one point you got incorrect - the constructors for the IO stream classes have a parameter String csn, by which you can specify the charset to use. See my answer for details. BTW, nice Newton in your DP! \$\endgroup\$ – Tamoghna Chowdhury May 5 '17 at 9:02
  • \$\begingroup\$ @RobAu that's not really necessary. See my answer. \$\endgroup\$ – Tamoghna Chowdhury May 5 '17 at 9:02
  • \$\begingroup\$ @TamoghnaChowdhury, thanks for the correction. The source code for generating the profile pic is at codegolf.stackexchange.com/a/2543/194 \$\endgroup\$ – Peter Taylor May 5 '17 at 9:55
8
\$\begingroup\$

Funny how nowbody seems to know the Files class...

I agree with the previous answers, that try-with-resources is a good thing to do, but to perform the task in a real simple way, use Files:

    // In reality, you practically always have your text in some kind of data structure:
    String[] text = {
        "The very first line of text.\n", // Note, that I added the newlines manually
        "----------------------------\n", // as Files.write does not do this for you
        "Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo ligula eget dolor.\n",
        "Donec quam felis, ultricies nec, pellentesque eu, pretium quis, sem. Nulla consequat massa quis enim.\n",
        "In enim justo, rhoncus ut, imperdiet a, venenatis vitae, justo.\n" };

    // Replaces the complete writing part:
    Files.write(Paths.get("filename"), Arrays.asList(text));

    // Replaces the complete reading part:
    Files.readAllLines(Paths.get("filename")).forEach(System.out::println);

(Please have a look at the API docs, there are also variants to explicitly set the character set https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html)

\$\endgroup\$
  • \$\begingroup\$ Great suggestion! Btw. you could use a nice lambda+map to append the '\n' after each line :) \$\endgroup\$ – RobAu May 5 '17 at 9:34
  • \$\begingroup\$ I'm working on mentioning it, but +1 for getting there earlier. \$\endgroup\$ – Tamoghna Chowdhury May 5 '17 at 10:17
  • \$\begingroup\$ There is a write overload that accepts encoding \$\endgroup\$ – Basilevs May 5 '17 at 11:58
7
\$\begingroup\$

I'll get started on the major things first - the I/O. Stylistic ideas will come later.

  1. Try-With-Resources

    If you're using Java 7+, use the try-with-resources statement. The syntax is similar to C#'s using statement:

    try (PrintWriter writer = new PrintWriter(new BufferedWriter(new FileWriter(currentDir + fileName)))) {
    // Do something using `writer`, it'll be closed automatically when it goes out of scope of the try-with-resources
    }
    catch (/* Whatever exceptions you need to handle*/){
    // Do something to handle the exception
    }
    // Look ma! No `finally`!
    

    Note that PrintWriter's methods don't throw any exceptions - you need to call checkError() on writer to find out if any errors occurred. Internal calls may produce exceptions which may propagate up to Printwriter, but that's not the general case. (Check the JavaDoc here.)

    The same for reading the file. Here my stylistic suggestion (2) should come in useful.

  2. Alternative Constructors and Encoding

    PrintWriter has a constructor PrintWriter(String fileName, String csn), which does exactly what you do manually. You could just provide it the file path and it'll work. No need to manually create BufferedWriter and FileWriter objects.

    What is that String csn in the second parameter? There's an overload of this constructor without the csn parameter - ideally, as @Peter Taylor said, you shouldn't use it. Why? "CSN" stands for "CharSet Name", or the output encoding. You'll generally want to pass in StandardCharsets.UTF_8.displayName(), which should be "UTF-8" (case insensitive) for csn. You can find StandardCharsets by importing java.nio.charset.StandardCharsets. Encoding can be a problem when you go multiplatform.

    Note that BufferedReader or FileReader do not have such a csn parameter in any of their constructors. What you would want to do here if have Java 7+ is to use the class java.nio.file.Files' newBufferedReader(Path path, Charset charset) method to create the BufferedReader with the specified charset.

    You might call it like this:

    // Ensure that the following classes have been imported properly
    // The IDE should help
    BufferedReader reader = Files.newBufferedReader(FileSystems.getDefault().getPath(currentdir, fileName), StandardCharsets.UTF_8);
    

    or, a slightly more succinct version:

    BufferedReader reader = Files.newBufferedReader(Paths.get(currentdir + fileName), StandardCharsets.UTF_8);
    

    The relevant JavaDocs are here:

    a. Files

    b. FileSystems

    c. FileSystem

    d. StandardCharsets

    e. Paths

  3. close() in finally

    If you aren't using Java 7 and above, the recommendation is to put all close() calls inside the finally statement after all the catches (the "Ye Olde" way). For this, your IO streams should be declared outside the try-catch-finally block.

    It could look like this:

    PrintWriter writer;
    try {
        // Using my suggestion from (2)
        writer = new PrintWriter(currentDir + fileName, "UTF-8");
    } catch(/* Whatever exceptions you need to handle*/) {
        // Do something to handle the exception
    } finally {
        writer.close();
    }
    

Stylistic suggestions:

  1. It is fair enough to close only the outermost stream - the close request will be propagated by the stream to any enclosed ones.

  2. You should make reading and writing as 2 different methods, readFile and writeFile. You may even want to parametrize them to take into account different files (file paths) and contents to be written.

  3. Your variable names are good and descriptive, nothing to fault there.

    Note that Java convention states that project names should be unique and involve a package structure involving the reversed domain name associated with the publisher of the software, i.e., your package structure should be something like com.mizech.modul. This ensures uniqueness in qualified names to a great extent.

    Another place where your naming scheme falters is in the naming of the exception objects. If we go with the scheme as object name = lowercase initials (first letter of each camelCased word) of type name, it should be fnfe for FileNotFoundException and ioe for IOException - but see point 9 below.

  4. I don't recommend the use of import static java.lang.System.*, as it may lead to namespace pollution, especially in an I/O oriented app like yours.

  5. I would really suggest you take a look at LineNumberReader - that very C-ish while loop is not really considered good Java. LineNumberReader will automatically keep track of the line number for you - otherwise, it's just a BufferedReader. You can get the current line number with it'sgetLineNumber()` method, however, note that the line number returned is 0-based. You might have to add 1 to the result to make it work like yours currently does.

  6. You really should look into java.nio.file.Files (JavaDoc linked above). It has utility methods for a lot of stuff - writing and reading files, file and directory management (copying, moving, deleting), etc. An exposition of the Java 8-enhanced properties of this class has already been provided by @mtj, and this answer's already pretty long, so I won't get into it here. Suffice it to say that most of what you've done is redundant and Java does indeed have a simpler and probably more efficient way of dealing with files - java.nio and subpackages therein.

  7. Please don't change the current directory for the reasons specified by @RobAu - it's very disorienting if your current working directory (CWD) gets changed if your app has been run from the terminal (using, say, java Sandbox, where Sandbox.class is in the CWD of the shell). If launched separately from the IDE using a terminal or console, the application's CWD will be the same as that of the launching console.

    If you absolutely have to do it, though, you can try System.setProperty("user.dir", "/foo/bar/");, where /foo/bar will be whatever directory you need to be the current working directory.

    If you just want to use a common relative root for all paths, you can rely on java.nio's Path objects' resolve method. They'll kind of do what you want - resolve the provided relative path string with the specified Path as the root.

  8. If you're handling all Exceptions the same way, then you might was well keep only the most general catch, catch (Exception e) (thanks @slowy!). Don't Repeat Yourself!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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