unnecessary code in Oracle example?

Discussion in 'Java' started by Jim Janney, Oct 9, 2013.

  1. Jim Janney

    Jim Janney Guest

    I'm reading up on Java 7's try-with-resource statement and looking at
    the tutorial at

    http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

    which includes the following code:

    > static String readFirstLineFromFileWithFinallyBlock(String path)
    > throws IOException {
    > BufferedReader br = new BufferedReader(new FileReader(path));
    > try {
    > return br.readLine();
    > } finally {
    > if (br != null) br.close();
    > }
    > }


    As a matter of habit, I always write that pattern as


    > static String readFirstLineFromFileWithFinallyBlock(String path)
    > throws IOException {
    > BufferedReader br = new BufferedReader(new FileReader(path));
    > try {
    > return br.readLine();
    > } finally {
    > br.close();
    > }
    > }


    on the theory that if you reach that point br can never be null, so the
    test is both redundant and confusing. On the other hand, I might be
    wrong. Is there a reason to test for null in the finally block?

    --
    Jim Janney
    Jim Janney, Oct 9, 2013
    #1
    1. Advertising

  2. Jim Janney

    Daniel Pitts Guest

    On 10/9/13 9:38 AM, Jim Janney wrote:
    > I'm reading up on Java 7's try-with-resource statement and looking at
    > the tutorial at
    >
    > http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
    >
    > which includes the following code:
    >
    >> static String readFirstLineFromFileWithFinallyBlock(String path)
    >> throws IOException {
    >> BufferedReader br = new BufferedReader(new FileReader(path));
    >> try {
    >> return br.readLine();
    >> } finally {
    >> if (br != null) br.close();
    >> }
    >> }

    >
    > As a matter of habit, I always write that pattern as
    >
    >
    >> static String readFirstLineFromFileWithFinallyBlock(String path)
    >> throws IOException {
    >> BufferedReader br = new BufferedReader(new FileReader(path));
    >> try {
    >> return br.readLine();
    >> } finally {
    >> br.close();
    >> }
    >> }

    >
    > on the theory that if you reach that point br can never be null, so the
    > test is both redundant and confusing. On the other hand, I might be
    > wrong. Is there a reason to test for null in the finally block?

    You are correct for this case, the other case comes from a block which
    uses multiple resources.

    MyResource r1 = null;
    MyResource r2 = null;

    try {
    r1 = openResource1();
    r2 = openResource2();
    } finally {
    if (r1 != null) r1.close();
    if (r2 != null) r2.close();
    }

    Of course, some close() methods can throw, which could break this idiom
    too. This is one reason why C++ forbids destructors from throwing.
    Daniel Pitts, Oct 9, 2013
    #2
    1. Advertising

  3. Jim Janney

    Stefan Ram Guest

    Daniel Pitts <> writes:
    >MyResource r1 = null;
    >MyResource r2 = null;
    >try {
    > r1 = openResource1();
    > r2 = openResource2();
    >} finally {
    > if (r1 != null) r1.close();
    > if (r2 != null) r2.close();
    >}


    Which also might be written as follows
    (execution would start at r() below,
    code was never syntax-checked nor tested).

    void use
    ( final MyResource r1,
    final MyResource r2 )
    { /* insert the code that needs r1 und r2 here */ }

    void r2
    ( final MyResource r1,
    final MyResource r2 )
    { try
    { use( r1, r2 ); }
    finally
    { r2.close(); }}

    void r1( final MyResource r1 )
    { try
    { r2( r1, openRessource2() ); }
    catch( final OpenResource2Exception exception )
    { /* add possible exception handling here */ }
    finally
    { r1.close(); }}}

    void r()
    { try
    { r1( openRessource1() ); }
    catch( final OpenResource1Exception exception )
    { /* add possible exception handling here */ }}

    The function calls are a trick that I have invented to
    bypass the need for non-final null-initialized variables
    when the reference of a resource just obtained is to be
    stored in a variable.

    I also do not have to compare the resources with null before
    closing them, since the functions with the close() calls are
    only called when the corresponding open calls did not throw.
    Stefan Ram, Oct 9, 2013
    #3
  4. Jim Janney

    Jim Janney Guest

    Daniel Pitts <> writes:

    > You are correct for this case, the other case comes from a block which
    > uses multiple resources.
    >
    > MyResource r1 = null;
    > MyResource r2 = null;
    >
    > try {
    > r1 = openResource1();
    > r2 = openResource2();
    > } finally {
    > if (r1 != null) r1.close();
    > if (r2 != null) r2.close();
    > }
    >
    > Of course, some close() methods can throw, which could break this
    > idiom too. This is one reason why C++ forbids destructors from
    > throwing.


    Thanks. But that looks like a good idiom to avoid. I prefer

    MyResource r1 = openResource();
    try {
    MyResource r2 = openResource2();
    try {
    // do something
    } finally {
    r2.close();
    }
    } finally {
    r1.close();
    }

    Not as pretty syntactically, perhaps, but much clearer semantically.

    --
    Jim Janney
    Jim Janney, Oct 9, 2013
    #4
  5. On 10/09/2013 08:19 PM, Daniel Pitts wrote:
    > On 10/9/13 9:38 AM, Jim Janney wrote:
    >> I'm reading up on Java 7's try-with-resource statement and looking at
    >> the tutorial at
    >>
    >> http://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
    >>
    >>
    >> which includes the following code:
    >>
    >>> static String readFirstLineFromFileWithFinallyBlock(String path)
    >>> throws IOException {
    >>> BufferedReader br = new BufferedReader(new FileReader(path));
    >>> try {
    >>> return br.readLine();
    >>> } finally {
    >>> if (br != null) br.close();
    >>> }
    >>> }

    >>
    >> As a matter of habit, I always write that pattern as
    >>
    >>
    >>> static String readFirstLineFromFileWithFinallyBlock(String path)
    >>> throws
    >>> IOException {
    >>> BufferedReader br = new BufferedReader(new FileReader(path));
    >>> try {
    >>> return br.readLine();
    >>> } finally {
    >>> br.close();
    >>> }
    >>> }

    >>
    >> on the theory that if you reach that point br can never be null, so the
    >> test is both redundant and confusing. On the other hand, I might be
    >> wrong. Is there a reason to test for null in the finally block?


    If you declare the variable "final" then there is no reason. The way
    you showed it "br" can actually become null.

    > You are correct for this case, the other case comes from a block which
    > uses multiple resources.
    >
    > MyResource r1 = null;
    > MyResource r2 = null;
    >
    > try {
    > r1 = openResource1();
    > r2 = openResource2();
    > } finally {
    > if (r1 != null) r1.close();
    > if (r2 != null) r2.close();
    > }
    >
    > Of course, some close() methods can throw, which could break this idiom
    > too.


    The proper (i.e. robust) way to do it would be to spend one try finally
    block per resource. That may not be the prettiest of idioms because of
    the indentation but it's robust.

    > This is one reason why C++ forbids destructors from throwing.


    Does it?

    $ cat -n x.cxx
    1
    2
    3 #include <iostream>
    4
    5 class Ex {
    6 };
    7
    8 class Foo {
    9 public:
    10 ~Foo() throw (Ex) {
    11 std::cout << "destructor" << std::endl;
    12 throw Ex();
    13 }
    14 };
    15
    16 int main() {
    17 try {
    18 Foo f;
    19 std::cout << "foo" << std::endl;
    20 }
    21 catch (Ex e) {
    22 std::cout << "caught" << std::endl;
    23 }
    24
    25 return 0;
    26 }
    27
    robert@fussel:~$ g++ x.cxx && ./a.out
    foo
    destructor
    caught

    Cheers

    robert
    Robert Klemme, Oct 9, 2013
    #5
  6. Jim Janney

    markspace Guest

    On 10/9/2013 12:08 PM, Jim Janney wrote:
    >
    > Thanks. But that looks like a good idiom to avoid. I prefer
    >
    > MyResource r1 = openResource();
    > try {
    > MyResource r2 = openResource2();
    > try {
    > // do something
    > } finally {
    > r2.close();
    > }
    > } finally {
    > r1.close();
    > }
    >
    > Not as pretty syntactically, perhaps, but much clearer semantically.
    >


    I don't like either one. Why not take advantage of the fact that
    resources are specified to be closed in the opposite order that they are
    declared in a try-with-resources?

    The following demonstrates this by closing the named streams in the
    order C-B-A:


    package quicktest;

    import java.io.IOException;
    import java.io.InputStream;

    public class TryResourcesTest
    {
    public static void main( String[] args ) throws Exception
    {
    try(
    MyTestStream a = new MyTestStream( "A" );
    MyTestStream b = new MyTestStream( "B" );
    MyTestStream c = new MyTestStream( "C" )
    ) {
    System.out.println( a.read()+b.read()+c.read() );
    }
    }
    }

    class MyTestStream extends InputStream {

    private final String name;

    public MyTestStream( String name ) {
    this.name = name;
    }

    @Override
    public int read() throws IOException {
    return 42;
    }

    @Override
    public void close() {
    System.out.println( getClass().getSimpleName()+
    ":"+name+" closed." );
    }
    }
    markspace, Oct 9, 2013
    #6
  7. Jim Janney

    Jim Janney Guest

    markspace <> writes:

    > On 10/9/2013 12:08 PM, Jim Janney wrote:
    >>
    >> Thanks. But that looks like a good idiom to avoid. I prefer
    >>
    >> MyResource r1 = openResource();
    >> try {
    >> MyResource r2 = openResource2();
    >> try {
    >> // do something
    >> } finally {
    >> r2.close();
    >> }
    >> } finally {
    >> r1.close();
    >> }
    >>
    >> Not as pretty syntactically, perhaps, but much clearer semantically.
    >>

    >
    > I don't like either one. Why not take advantage of the fact that
    > resources are specified to be closed in the opposite order that they
    > are declared in a try-with-resources?
    >
    > The following demonstrates this by closing the named streams in the
    > order C-B-A:
    >
    >
    > package quicktest;
    >
    > import java.io.IOException;
    > import java.io.InputStream;
    >
    > public class TryResourcesTest
    > {
    > public static void main( String[] args ) throws Exception
    > {
    > try(
    > MyTestStream a = new MyTestStream( "A" );
    > MyTestStream b = new MyTestStream( "B" );
    > MyTestStream c = new MyTestStream( "C" )
    > ) {
    > System.out.println( a.read()+b.read()+c.read() );
    > }
    > }
    > }
    >
    > class MyTestStream extends InputStream {
    >
    > private final String name;
    >
    > public MyTestStream( String name ) {
    > this.name = name;
    > }
    >
    > @Override
    > public int read() throws IOException {
    > return 42;
    > }
    >
    > @Override
    > public void close() {
    > System.out.println( getClass().getSimpleName()+
    > ":"+name+" closed." );
    > }
    > }


    Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    like a win all around.

    --
    Jim Janney
    Jim Janney, Oct 9, 2013
    #7
  8. Jim Janney

    markspace Guest

    On 10/9/2013 1:21 PM, Jim Janney wrote:
    > Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    > like a win all around.
    >


    For Java 5+, with varargs you could do (not compiled):

    public class MyIoUtils {

    public static void closeAll( Closeable ... toClose ) {
    for( Closeable closeMe : toClose )
    try {
    if( closeMe != null ) closeMe.close();
    } catch( IOException ex ) {
    Logger.getLogger( "MyIoUtils" ).log( ex );
    }
    }
    }

    and get a somewhat cleaned up try catch

    OutPutStream io1 = null;
    BufferedStream bs = null;
    try {
    io1 = new OutputStream( ...
    bs = new BufferedStream( io1, ...
    // do stuff
    }
    finally {
    MyIoUtils.closeAll( bs, io1 );
    }

    Mostly I don't care for deep nesting, I think it reads poorly, and I
    think it can also be hard to trace out for a programmer reading the
    source. Giving the concept of closing a lot of objects a name like
    "closeAll" promotes literate programming, imo.
    markspace, Oct 9, 2013
    #8
  9. Jim Janney

    Jim Janney Guest

    markspace <> writes:

    > On 10/9/2013 1:21 PM, Jim Janney wrote:
    >> Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    >> like a win all around.
    >>

    >
    > For Java 5+, with varargs you could do (not compiled):
    >
    > public class MyIoUtils {
    >
    > public static void closeAll( Closeable ... toClose ) {
    > for( Closeable closeMe : toClose )
    > try {
    > if( closeMe != null ) closeMe.close();
    > } catch( IOException ex ) {
    > Logger.getLogger( "MyIoUtils" ).log( ex );
    > }
    > }
    > }
    >
    > and get a somewhat cleaned up try catch
    >
    > OutPutStream io1 = null;
    > BufferedStream bs = null;
    > try {
    > io1 = new OutputStream( ...
    > bs = new BufferedStream( io1, ...
    > // do stuff
    > }
    > finally {
    > MyIoUtils.closeAll( bs, io1 );
    > }
    >
    > Mostly I don't care for deep nesting, I think it reads poorly, and I
    > think it can also be hard to trace out for a programmer reading the
    > source. Giving the concept of closing a lot of objects a name like
    > "closeAll" promotes literate programming, imo.


    Mmm... you're complicating the code with extra assignments and tests,
    just to avoid one layer of nesting. I like the version with two
    try-finally blocks, partly because the lexical structure reflects what's
    happening in the code: two independent cleanups. But the Java 7 version
    is better.

    --
    Jim Janney
    Jim Janney, Oct 10, 2013
    #9
  10. Jim Janney

    Arivald Guest

    W dniu 2013-10-10 04:56, Jim Janney pisze:
    > markspace <> writes:
    >
    >> On 10/9/2013 1:21 PM, Jim Janney wrote:
    >>> Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    >>> like a win all around.
    >>>

    >>
    >> For Java 5+, with varargs you could do (not compiled):

    [...]
    >
    > Mmm... you're complicating the code with extra assignments and tests,
    > just to avoid one layer of nesting. I like the version with two
    > try-finally blocks, partly because the lexical structure reflects what's
    > happening in the code: two independent cleanups. But the Java 7 version
    > is better.
    >


    I wonder how Java 7 version (try-with-resources) handle exceptions...
    For example, if all 3 streams throw exception from close(), will it
    guarantee to close all of them? And how to handle such exceptions?

    --
    Arivald
    Arivald, Oct 10, 2013
    #10
  11. Jim Janney

    Sebastian Guest

    Am 10.10.2013 06:42, schrieb Arivald:
    > W dniu 2013-10-10 04:56, Jim Janney pisze:
    >> markspace <> writes:
    >>
    >>> On 10/9/2013 1:21 PM, Jim Janney wrote:
    >>>> Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    >>>> like a win all around.
    >>>>
    >>>
    >>> For Java 5+, with varargs you could do (not compiled):

    > [...]
    >>
    >> Mmm... you're complicating the code with extra assignments and tests,
    >> just to avoid one layer of nesting. I like the version with two
    >> try-finally blocks, partly because the lexical structure reflects what's
    >> happening in the code: two independent cleanups. But the Java 7 version
    >> is better.
    >>

    >
    > I wonder how Java 7 version (try-with-resources) handle exceptions...
    > For example, if all 3 streams throw exception from close(), will it
    > guarantee to close all of them? And how to handle such exceptions?
    >

    Why don't you modify markspace's example to see for yourself, e. g.

    @Override
    public void close() throws IOException {
    System.out.println( "Trying to close "
    + getClass().getSimpleName()+":"+name );
    throw new IOException("CloseError " + name);
    }

    Then read what the Javadocs have to say about suppressed exceptions.

    -- Sebastian
    Sebastian, Oct 10, 2013
    #11
  12. Jim Janney

    Arivald Guest

    W dniu 2013-10-10 07:47, Sebastian pisze:
    > Am 10.10.2013 06:42, schrieb Arivald:
    >> W dniu 2013-10-10 04:56, Jim Janney pisze:
    >>> markspace <> writes:
    >>>
    >>>> On 10/9/2013 1:21 PM, Jim Janney wrote:
    >>>>> Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    >>>>> like a win all around.
    >>>>>
    >>>>
    >>>> For Java 5+, with varargs you could do (not compiled):

    >> [...]
    >>>
    >>> Mmm... you're complicating the code with extra assignments and tests,
    >>> just to avoid one layer of nesting. I like the version with two
    >>> try-finally blocks, partly because the lexical structure reflects what's
    >>> happening in the code: two independent cleanups. But the Java 7 version
    >>> is better.
    >>>

    >>
    >> I wonder how Java 7 version (try-with-resources) handle exceptions...
    >> For example, if all 3 streams throw exception from close(), will it
    >> guarantee to close all of them? And how to handle such exceptions?
    >>

    > Why don't you modify markspace's example to see for yourself, e. g.
    >
    > @Override
    > public void close() throws IOException {
    > System.out.println( "Trying to close "
    > + getClass().getSimpleName()+":"+name );
    > throw new IOException("CloseError " + name);
    > }
    >
    > Then read what the Javadocs have to say about suppressed exceptions.
    >
    > -- Sebastian



    I read docs about suppressed exceptions. And of course I can run test.
    But still I wonder how it should behave, if there is more than one
    close-able resource, two of them raise exceptions, and there was no
    exception in main block (so no suppression).

    Can anyone point me to relevant doc? Google is not very helpful.

    --
    Arivald
    Arivald, Oct 10, 2013
    #12
  13. Jim Janney

    Jim Janney Guest

    Arivald <arivald_@AT_interia_DOT_.pl> writes:

    > W dniu 2013-10-10 07:47, Sebastian pisze:
    >> Am 10.10.2013 06:42, schrieb Arivald:
    >>> W dniu 2013-10-10 04:56, Jim Janney pisze:
    >>>> markspace <> writes:
    >>>>
    >>>>> On 10/9/2013 1:21 PM, Jim Janney wrote:
    >>>>>> Yes, in Java 7 you can have clean syntax and clean semantics. Looks
    >>>>>> like a win all around.
    >>>>>>
    >>>>>
    >>>>> For Java 5+, with varargs you could do (not compiled):
    >>> [...]
    >>>>
    >>>> Mmm... you're complicating the code with extra assignments and tests,
    >>>> just to avoid one layer of nesting. I like the version with two
    >>>> try-finally blocks, partly because the lexical structure reflects what's
    >>>> happening in the code: two independent cleanups. But the Java 7 version
    >>>> is better.
    >>>>
    >>>
    >>> I wonder how Java 7 version (try-with-resources) handle exceptions...
    >>> For example, if all 3 streams throw exception from close(), will it
    >>> guarantee to close all of them? And how to handle such exceptions?
    >>>

    >> Why don't you modify markspace's example to see for yourself, e. g.
    >>
    >> @Override
    >> public void close() throws IOException {
    >> System.out.println( "Trying to close "
    >> + getClass().getSimpleName()+":"+name );
    >> throw new IOException("CloseError " + name);
    >> }
    >>
    >> Then read what the Javadocs have to say about suppressed exceptions.
    >>
    >> -- Sebastian

    >
    >
    > I read docs about suppressed exceptions. And of course I can run
    > test. But still I wonder how it should behave, if there is more than
    > one close-able resource, two of them raise exceptions, and there was
    > no exception in main block (so no suppression).
    >
    > Can anyone point me to relevant doc? Google is not very helpful.


    http://blog.eyallupu.com/2013/03/try-catch-resource-and.html

    Something to think about when reworking old code, if anyone has time for
    that.

    --
    Jim Janney
    Jim Janney, Oct 10, 2013
    #13
  14. Jim Janney

    Guest

    >
    >
    > > This is one reason why C++ forbids destructors from throwing.

    >
    > Does it?
    >


    True, it isn't forbidden directly, but the "rule" is a clear
    consequence of the way the language works, where throwing
    destructors cause most programs to misbehave.

    The standard does explicitly warn that it's a bad idea:

    15.2.3: The process of calling destructors for automatic objects
    constructed on the path from a try block to a throw-expression is
    called "stack unwinding." If a destructor called during stack
    unwinding exits with an exception, std::terminate is called
    (15.5.1). [ Note: So destructors should generally catch exceptions and
    not let them propagate out of the destructor. —end note ]

    But this is a java forum...
    , Oct 10, 2013
    #14
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Dominik Jain
    Replies:
    9
    Views:
    8,530
    Dominik Jain
    Aug 4, 2005
  2. Steffen Beyer
    Replies:
    3
    Views:
    750
    Richard Tobin
    Nov 2, 2004
  3. Richard Clarke
    Replies:
    2
    Views:
    336
    Bob Hairgrove
    Oct 18, 2004
  4. Golan
    Replies:
    1
    Views:
    338
    Flash Gordon
    Apr 5, 2005
  5. Feyruz
    Replies:
    4
    Views:
    2,158
    Sherm Pendley
    Oct 14, 2005
Loading...

Share This Page