marvk/badcode_graph.md

## badcode_graph.md

      
    Raw
  

              badcode_graph.md
            
          
    So, whats wrong with the following start to a constructor of a Graph class?
public Graph(String fileName) {
    File file = null;
    try {
        file = new File(fileName);
    } catch (Exception e) {
        // TODO: handle exception
    }
    if(file==null){
        System.out.println("Graph1.csv could not be found");
        return;
    }
    // Parsing code
}
The most obvious thing is that the Exception thrown by File::new is ignored and something is printed to System.out instead. So lets fix that:
public Graph(String fileName) {
    File file = null;
    try {
        file = new File(fileName);
    } catch (Exception e) {
        System.err.println("Graph1.csv could not be found");
        e.printStackTrace();
        return;
    }
    // Parsing code
}
Note how I also replaced System.out with System.err, because we're actually printing an error.
Now, why does the error message not specify the file name but Graph1.csv instead? That is certainly wrong: The filename isn't always Graph.csv since we pass it to the Constructor.
public Graph(String fileName) {
    File file = null;
    try {
        file = new File(fileName);
    } catch (Exception e) {
        System.err.println(fileName + " could not be found");
        e.printStackTrace();
        return;
    }
    // Parsing code
}
Much better. But now I hear you say, File::new doesn't throw any exception besides NullPointerException? So no need for catching Exception, which you should almost never do anyways. It's much better to catch the most concise Exception possible. However, you should also never catch a NullPointerException, since instead, you can and should check for null. So, lets change the code based on what I just wrote:
public Graph(String fileName) {
    File file = new File(fileName);
    
    if (file == null) {
        System.err.println(fileName + " could not be found");
        return;
    }

    // Parsing code
}
Way better still. We still have some problems though. When the path doesn't exist, we're leaving the created Graph object in an inconsistent state. So instead of just returning, lets throw an Exception ourselves to signify that the caller passed a bad parameter:
public Graph(String fileName) throws FileNotFoundException {
    File file = new File(fileName);

    if (file == null) {
        throw new FileNotFoundException(fileName + " could not be found");
    }

    // Parsing code
}
That's better, but of course my IDE is already warning me:

file can obviously never be null, since at that point, File::new returned just fine and and a constructor can never return null. Lets check if the file exists instead:
public Graph(String fileName) throws FileNotFoundException {
    File file = new File(fileName);

    if (!file.exists()) {
        throw new FileNotFoundException(fileName + " could not be found");
    }

    // Parsing code
}
We're getting close. Now, instead of passing a String to the constructor, we could pass a File to make the contract of the constructor clearer. This eliminates people passing the wrong String object by accident, for example.
public Graph(File file) throws FileNotFoundException {
    if (!file.exists()) {
        throw new FileNotFoundException(file + " could not be found");
    }

    // Parsing code
}
Much better still! We're going to make a few more adjustments though. Java 7 introduced Path, which has a number of advantages over File that I don't want to get into to deeply, but you can certainly find them if you search on Google. And even if your implementation requires a File (Which in this case, it did since they used a Scanner instead of, for example Files::readAllLines), you can easily convert a Path back to a file with Path::toFile.
public Graph(final Path path) throws FileNotFoundException {
    if (!Files.exists(path)) {
        throw new FileNotFoundException(path + " could not be found");
    }

    // Parsing code
}
Excellent. You will notice that I also made the parameter path final. This is something I strive to do on all my variables, fields and parameters, as it makes your code less prone to errors. While it may seem like clutter (and I wish java would have introduced a let when they introduced var), you avoid things like, for example, accidentally assigning to a parameter instead of a field. Immutable objects are also much easier to handle in a concurrent context.
Thanks for reading, I hope that you found this interesting or even learned something.