views:

468

answers:

7

how can i rewrite the following method using try and catch statements instead of the throws clause in the method header:

public String getInput(String filename) throws Exception
{
    BufferedReader infile = new BufferedReader (new FileReader(filename));
    String response = infile.readLine();
    infile.close();

    return response:
}
+4  A: 

Your answer can be found here.

dangerfan
Just make sure you read all of the pages for that... otherwise you will miss some important things.
TofuBeer
i have a problem ,am putting a class it says it cant find BufferedReader method.
asong84, just import it.
BalusC
-1 Although the answer is correct, it would be more helpful to pin point the relevant section from that document.
OscarRyz
+2  A: 
public String getInput(String filename)
{
    BufferedReader infile = null;
    try {
       infile = new BufferedReader (new FileReader(filename));
       String response = infile.readLine();
       return response;
    } catch (IOException e) {
       // handle exception here
    } finally {
       try { infile.close(); } catch (IOException e) { }
    }
    return null;
}
Pablo Santa Cruz
NEVER catch Exception unless you have to. Any general purpose API that throws Exception is broken. You should be doing catch(IOException e).
TofuBeer
Also this code is wrong... you must put the close in a finally block or it will fail to close (and depending on how things are implemented leak the file handle in the OS) if the readLine throws an exception.
TofuBeer
@Tofu - Agree. No API should throw or catch Exception, but that's what the person who posted the question was asking for.
Pablo Santa Cruz
no excuse for giving bad advice :-) (but at least I see your motivation).
TofuBeer
Slight tweak too..."infile.close()" won't compile as written because the BufferedReader is declared inside the "try" block rather than at the method level. I also agree with @TofuBeer...change the catch to IOException not Exception.
Brent Nash
when closing `infile` it will be `null` when the file is not found: NullPointerException...
Carlos Heuberger
A: 

Something like this should do it:

public String getinput(String filename) 
{
    BufferedReader infile = null;
    String response = null;
    try {
        infile = new BufferedReader(new FileReader(filename));
        response = infile.readLine();
    } catch (IOException e) {
        //handle exception
    } finally {
        try {
            if (infile != null)
              infile.close();
        } catch (IOException e) {
            //handle exception
        }
    }
    return response;
}

Note you should add the finally clause as stated by others, as you might end up with memleaks if the BufferedReader is not closed properly, because of an exception.

Sune Rievers
Apart from the NPE - again, try two try statements.
Tom Hawtin - tackline
also **String** should be used instead of **string**. :-)
Pablo Santa Cruz
You're both right, fixed!
Sune Rievers
do not catch Exception... catch IOException (see my comment to Pablo Santa Cruz).
TofuBeer
Right, didn't have an IDE ready to help me write it :)
Sune Rievers
potential NPE doing `infile.close()` (e.g. file not found)
Carlos Heuberger
Good point ;) ...
Sune Rievers
+9  A: 

Try and catch are used to gracefully handle exceptions, not hide them. If you are calling getinput() wouldn't you want to know if something went wrong? If you wanted to hide it I suppose you could do something like

public String getInput(String file) {
    StringBuilder ret = new StringBuilder();
    String buf;
    BufferedReader inFile = null;

    try {
        inFile = new BufferedReader(new FileReader(filename));
        while (buf = inFile.readLine())
            ret.append(buf);
    } catch (FileNotFoundException e) {
        ret.append("Couldn't find " + file);
    } catch (IOException e) {
        ret.append("There was an error reading the file.");
    } finally {
        if (inFile != null) {
           try {
              inFile.close();
           } catch (IOException aargh) {
              // TODO do something (or nothing)
           }
        }
    }

    return ret.toString();
}

It's worth noting that you want to be catching the exceptions individually. Blindly catching Exception as some answers have suggested is a bad idea. You don't want to catch things you never saw coming to handle something you did. If you want to catch exceptions you never saw coming you need to log it and gracefully display an error to the user.

jcm
close - just catch the last `IOException` finally and you're done
Andreas_D
Most complete answer. only missing the catch for IOException inside the finally block.
ChadNC
Now it's complete.
Andreas_D
Ah, thanks Andreas.
jcm
+4  A: 

Here's a good SO thread on exception handling strategy:

http://stackoverflow.com/questions/1335821/critique-my-exception-handling-strategy/1336113#1336113

A couple other thoughts:

  1. Be careful about catching things and hiding them. In this case, I would say it's actually better to use "throws" because you're informing the caller of your method that something went wrong and you couldn't keep up your end of the bargain (returning a valid response). Though I would say "throws IOException" rather than just plain old "Exception". If you're going to bother catching the exception, do something constructive with it, don't just catch it for the sake of catching it.

  2. You might want to use a try/catch/finally when you're dealing with file I/O and close your file in the finally clause.

As far as the code goes, look at @Pablo Santa Cruz's and read the comments. My code would be pretty similar.

Brent Nash
A: 

I would write it like this:

public String getInput(String filename) {
    BufferedReader infile = null;
    String response = "";
    try { 
        infile = new BufferedReader (new FileReader(filename));
        response = infile.readLine();
    } catch( IOException ioe ) {
        System.err.printf("Exception trying to read: %s. IOException.getMessage(): %s",
                           filename, ioe.getMessage() );
    } finally {
        if( infile != null ) try {
            infile.close();
        } catch( IOException ioe ){}
    }
    return response;
}

In this particular case, if the exception is thrown the empty string "" is good for me. This is not always the case.

OscarRyz
I would prefer `null` above `""`. That's easier to deal with at higher levels.
BalusC
1) print the StackTrace of the IOException, it's (almost) always helpfull ..... 2) the empty catch is not a good example, can hide important errors
Carlos Heuberger
A: 

This is an adaptation of jcms answer - did it because I didn't like the exception handling in his solution...

We have to decide what to do in case of an exception. That's usually covered in some requirements. Logging is a good idea, but we still have to do something. At least I'd never use the return value to report file content as well as error messages. That's pretty hard to decode for the receiver. One could throw an exception - the usual way, if a missing file or an IO error is an exceptional situation. Or, and that's my suggestion, we define a small class to return both file content and error state:

public class FileContent {
  private String fileContent = null;
  private Throwable error = null;

  public FileContent(String fileContent, Throwable error) {
    this.error = error;
    this.fileContent = fileContent;      
  }

  // getters for all fields (no setters!)

  public boolean isValid() {
    return (error == null);
  }
}

and the getInput() method will be like this:

public FileContent getInput(final String fileName) {
    final StringBuilder fileContentBuilder = new StringBuilder();
    String buffer = null;
    BufferedReader reader = null;
    Throwable error = null;

    try {
        reader = new BufferedReader(new FileReader(fileName));
        while (buffer = reader.readLine()) {
            fileContentBuilder.append(buffer);
        }
    } catch (FileNotFoundException e) {
        error = new RuntimeException("Couldn't find " + fileName, e);
    } catch (IOException e) {
        error = new RuntimeException("There was an error reading the file.", e);
    } finally {
        if (inFile != null) {
           try {
              inFile.close();
           } catch (IOException e) {
              error = new RuntimeException(
                         "Couldn't close reader for file " + fileName, e);
           }
        }
    }

    return new FileContent(fileContentBuilder.toString(), error);
}

public void useGetInput() {
   FileContent content = getInput("your/path/to/your/file");
   if (content.isValid()) {
     System.out.println(content.getFileContent());
   } else {
     content.getError().printStackTrace();
   }
}

(Improvements; instead of using RuntimeException as a Wrapper, we could define our own exception type)

Andreas_D
if there is a previous error, you are hiding it in case of an error closing the reader. I would not change the `error` variable if it is not null.
Carlos Heuberger
@Carlos, yes, I know and thought about it, but then I decided to keep it simple in that area. Normally, the FileContent should hold a Collection of errors. There's only one chance, that an error is hidden: that's when we had an exception and get another one while closing the stream. That's very, very unlikely and that's why implemented it 'the short way'.
Andreas_D