views:

121

answers:

6

Hey guys,

I'm currently working on a project and I've come upon a bit of a head scratcher. I've been programming in Java for about two years, and I've covered exceptions, but never properly understood them.

In my current situation I've got my main method which initializes a class

WikiGraph wiki = wiki = new WikiGraph(getFile());

The Wikigraph constructor takes a filename which I get via a DialogBox in the getFile() method.

The constructor for wikigraph then calls a method called loadfile(filename) which attemps to load and parse the file given.

The loadfile() method is where I will throw an IncorrectFileTypeError.

My question is, where should I handle this?

At the moment I catch it in the loadfile() method

try {
    //load file
} catch (IncorrectFileTypeError e){
    System.out.println("Incorrect file type: "+filename+": "+e.getMessage());
    throw new IncorrectFileTypeError();
}

But I also catch it at the WikiGraph initialization like so:

while(wiki==null){                          //While There is no valid file to add to the wikigraph
    try {
        wiki = new WikiGraph(getFile());    //Try to load file  
    } catch (IncorrectFileTypeError e) {    //If file cannot be loaded
        JOptionPane.showMessageDialog(null, "Incorrect File Type Given. Please Choose another file."); //Display error message
        getFile();                          //Prompt user for another file
    }
}

Now is the way I've handled the error the correct/best way? Or should it be handled elsewhere, such as in the getFile() method?

EDIT: I suppose I should make the file issue a bit clearer. The File extension is not what the IncorrestFileTypeError is based on, and thus it may be a misleading error name. The file given may have pretty much any extension, its the contents of that must be well formed.

A: 

I don't think it makes sense to catch it in loadfile. You can still write a log message (use System.err), just do it before throwing the exception. You're on the right track with catching the exception where you can do something about it (in this case prompting the user again).

Matthew Flaschen
+1 for "catching the exception where you can do something about it", but -1 for "You can still write a log message". Please, *please* to not both log and throw (see e.g. http://codemonkeyism.com/7-good-rules-to-log-exceptions/, "don't log and rethrow"). The exception *will* be handled somewhere, and there the decision to log should be made. Otherwise you'll get two log entries for one error, which can be extremely confusing.
sleske
I never said rethrow. I said log "before throwing the exception". In this case, I think loadfile is the sole appropriate level for logging.
Matthew Flaschen
+2  A: 

One of the main benefits of exceptions is that they give you convenient means to handle an exception far from where it occurs. In languages that don't support it, you would have had to have each call on the path check the return value, break and return, so that you are manually propagating.

It makes sense to handle the exception where you feel you can either recover from it or best report it and cancel the operation.

My understanding is that the interaction starts with UI input and that if the file type is inappropriate, you cannot really recover. Therefore, I would have whatever UI task that initiated the file open catch the exception, report to the user that an error occurred, and either cancel or ask for a different input.

On a second reading, it seems like you are opening the dialog after you've already started trying to create the graph. I personally like to isolate UI interaction. I would therefore personally first handle all UI input, get the expected file name, verifies that it meets my needs, report to the user if not, and only continue further if the file is ok, then reporting only critical and unexpected errors.

Uri
+3  A: 

In this case, I would avoid using exceptions as your primary method of dealing with incorrect file types. If an exception can regularly be triggered by user input, it really shouldn't be treated as an exception. Here's the approach I would take:

  1. Check the file type and alert the user if the file type is invalid, through standard control flow. You might consider a static method on WikiGraph like IsFileValid(filename)
  2. In the WikiGraph initialization, throw an exception if the file type is invalid.

This approach gives you the best of both worlds - you have the opportunity to alert users when invalid data is provided, while still ensuring from WikiGraphs perspective that the information provided is accurate. It gives developers working against a WikiGraph a way to ensure their input is valid without necessarily requiring exception handling.

Ryan Brunner
Ah yes but my file extension is not set. I was given a sample .idx file to process. It could be .txt, or anything, so I can't check based on filename extension, only on the contents of the file. The isFileValid() method would then have to parse the file to check that its valid, so since that's happening in the loadfile() method already shouldn't I just catch it there.
Chris Salij
Yes, unfortunately those kind of problems can come up. I would still recommend failing at the UI level without exceptions if there's a relatively simple check that can be done on the file (i.e. checking the header), but for more complicated checks you may need to check in loadfile())
Ryan Brunner
@Chris: I disagree. If the file is invalid, obviously the WikiGraph instance he's trying to create cannot be properly initialized. Allowing partly-initialized instances is very dangerous. So I'd just let the constructor declare the exception, then put a try-catch around the constructor. The try would of course still prompt the user.
sleske
@sleske: Actually that's a very good point. I hadn't considered that. So I would throw the error at the loadFile() method, but I would catch it at the Constructor and deal with it at the Initialization?
Chris Salij
@Chris: Yes, the important point being to catch the exceptoin *outside* the constructor. That way (at least in Java) you will not get an incompletely-initialized instance.
sleske
A: 

If the problem is that the user has choosen a file of the wrong type then I don't think you should let the code proceed that far at all. This does not really seem like it should be an Exception at all but more a path in the program. If the user has selected an incorrect file type then he should be presented with the choice of choosing the file again. Obviously you should limit the choices in the JFileChooser to files of the correct type if it is as easy as looking at the extension.

willcodejavaforfood
A: 

I think this is the type of situation that checked exceptions are designed for (whether you agree with them or not.) I assume IncorrectFileTypeError extends Error? Extending RuntimeException would be more appropriate, and extending IOException would be better still, because this kind of error can be triggered by valid user input (i.e. typing the name of a file that exists but has the wrong type.)

finnw
A: 

I believe this type of error (IncorrectFileTypeError) can be resolved in the User Input Form avoiding an unnecessary roundtrip to filesystem service layer.

João Paraná