Nested Redundant try catch blocks



  • public SqlDataReader DoSomething()
    {
        try
        {
            // Some code

            try
            {
                // Some more code
            }
            catch (Exception ex)
            {
                throw ex;
            }
            finally
            {
                // Some class that implements IDisposable
                dataReader.Close();
            }
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

     

    For the love of God, WHY!!??

    Note: this pattern is repeated throughout the code base.



  •  Some ridiculous code standard mandate that all methods must have a trycatch, and because this guy (very sanely!) wanted a finally, he just nested it?



  • @dhromed said:

     Some ridiculous code standard mandate that all methods must have a trycatch, and because this guy (very sanely!) wanted a finally, he just nested it?

     

     

    But you could just wrap the whole thing inside a 'using' statement and it'll have the exact same result.

     

    using(SqlDataReader reader = //Code to get DataReader)

    {

        // Some code here

        return reader;

    }

     



  • @Sunday Ironfoot said:

    But you could just wrap the whole thing inside a 'using' statement and it'll have the exact same result.

    using(SqlDataReader reader = //Code to get DataReader)

    {

        // Some code here

        return reader;

    }

     

     

    I have no idea what that code does.

     



  • using statement is for types that implement IDisposable, the Dispose() method will be called (and thus resources cleaned by) at the end of the using code block, even if an exception is raised, so the code:

     

    try
    {
        SqlDataReader reader = // Code to get DataReader

        // Some code here

        return reader;
    }
    catch(Exception ex)
    {
        throw ex;
    }
    finally
    {
        reader.Dispose();
    }

     

    Is exactly the same as:

     

    using(SqlDataReader reader = // Code to get DataReader)
    {
        // Some code here

        return reader;
    }



  • @dhromed said:

    I have no idea what that code does.
    If you have no idea what the code does because you come from a Java background, it's worth noting that a virtually identical syntax made it into Java 7:

    try (Resource res = getResource()) {
        // Do stuff with res.
    }
    

    P.S.: try-finally < using block < lambda callbacks < custom monads.


  • ♿ (Parody)

    @Sunday Ironfoot said:

    using statement is for types that implement IDisposable

    Nested try-catch-finallies seem WTFy to me, but if you're saying this is a WTF because he could have used the syntactic sugar of using, then this is more of a mug at best, and pedantic dickweedery to be sure.



  • He must have never heard of "using".  Or, he is "using".  Whatever.

    Addendum:

    At first, before I really read the code, I had the fleeting impression that he was calling Dispose() from another class's finalizer, which is a no-no, since finalizers run in no predictable order.



  • [ Got posted twice for some reason. Mod, please delete this. ]



  • @boomzilla said:

    ....but if you're saying this is a WTF because he could have used the syntactic sugar of using, then this is more of a mug at best, and pedantic dickweedery to be sure.

     No! The WTF is that the try/catch blocks are completely redundant because he's just rethrowing the exceptions up the call stack. I just got sidetracked by the using stuff.



  • Oh, bugger! Last week I wrote some code with about 5 nested try/catch blocks in it.......


    In my defence, last week wasn't my best week ever, I will plead "distracted by personal crap"!!


    Right, I'm off right now to destroy some nests...



  • I've used this sort of paradigm before for PreparedStatements/ResultSets. Though, I admit that I am not an expert in Java. Basically it went something like:

    Connection conn = null;
    PreparedStatement ps = null;
    ResultSet rs = null;
    
    try {
        conn = ...
        ps = conn.prepareStatement(...);
        ...
        rs = ps.execute(); // I could be mistaken about the exact method name, but you get the idea
    } catch (SqlException se) {
        // Do something with the exception, like log it.
    } finally {
        try {
            if (rs != null) rs.close();
            if (ps != null) ps.close();
            if (conn != null) conn.close();
        } catch (SqlException se) {
            // Sometimes ignore this exception, depending on the context
        }
    }

    In what little defense that I have - if any, I wasn't the original author of the code, I just kept using it for consistency (and a lack of better understanding).


  • ♿ (Parody)

    @Sunday Ironfoot said:

    No! The WTF is that the try/catch blocks are completely redundant because he's just rethrowing the exceptions up the call stack. I just got sidetracked by the using stuff.

    Very good. Carry on.



  • @dohpaz42 said:

    I've used this sort of paradigm before for PreparedStatements/ResultSets. Though, I admit that I am not an expert in Java. Basically it went something like:

    Connection conn = null;
    PreparedStatement ps = null;
    ResultSet rs = null;
    

    try {
    conn = ...
    ps = conn.prepareStatement(...);
    ...
    rs = ps.execute(); // I could be mistaken about the exact method name, but you get the idea
    } catch (SqlException se) {
    // Do something with the exception, like log it.
    } finally {
    try {
    if (rs != null) rs.close();
    if (ps != null) ps.close();
    if (conn != null) conn.close();
    } catch (SqlException se) {
    // Sometimes ignore this exception, depending on the context
    }
    }

    In what little defense that I have - if any, I wasn't the original author of the code, I just kept using it for consistency (and a lack of better understanding).

    I understand that in JDBC (and possibly other languages) database 'handles' don't clean up after themselves, so this code actually SHOULD be more nested -- each open/close pairing should have its closing in its own finally block. In your code, above, if the rs.close() throws an exception, the ps and conn won't be closed.

    Connectino conn;
    PreparedStatement ps;
    ResultSet rs;
    try {
      conn = DriverManager.getConnection(...);
      try {
        ps = conn.prepareStatement(...);
        try {
          rs = ps.executeQuery();
          while(rs.next()) {
            // work with the ResultSet
          }
        } catch (SQLException e) {
          // log the error / set error condition
        } finally {
          try {
            if (rs != null) rs.close();
          } catch (SQLException e) {
            // log the error and continue
          }
        }
      } catch (SQLException e) {
         // log the error / set error condition
      } finally {
         try {
           if (ps != null) ps.close();
         } catch (SQLException e) {
           // log the error and continue
         }
      }
    } catch (SQLException e) {
      // log the error / set error condition
    } finally {
      try { 
        if (conn != null) conn.close();
      } catch (SQLException e) {
        // log the error and continue
      }
    }
    Or something ... since exceptions act like gotos, you'd be skipping over the cleanup code. (Oddly enough, Wikipedia seems to have a good writeup on JDBC with similar code to the above...) TRWTF is having to handle exceptions on .close() actions ... I mean ... seriously ... if the thing won't close, what else is there to do?


  • It could be left-over debug code, when he wanted to put a breakpoint when an exception happens (without enabling the "catch all exceptions on first chance"). The first block breaks before the finally {} and the second one after that.



  • @MeesterTurner said:

    Oh, bugger! Last week I wrote some code with about 5 nested try/catch blocks in it.......

    In my defence, last week wasn't my best week ever, I will plead "distracted by personal crap"!!

    Right, I'm off right now to destroy some nests...

    I almost wrote a nested try-catch yesterday (for the first time) before I woke up and realized that my inner exceptions were of a different type than my outers. (For Web and IO exceptions, I needed to retry a few times, but for the inner exception - which could be anything since I'm calling a user delegate, I store it and flag it for review by the calling thread when it joins later).



  • @Sunday Ironfoot said:

    No! The WTF is that the try/catch blocks are completely redundant because he's just rethrowing the exceptions up the call stack. I just got sidetracked by the using stuff.
    They're not entirely redundant... They reset the stack trace of the caught-rethrown expections, therefore hiding where the exception really originated.



  • @Zecc said:

    @Sunday Ironfoot said:

    No! The WTF is that the try/catch blocks are completely redundant because he's just rethrowing the exceptions up the call stack. I just got sidetracked by the using stuff.
    They're not entirely redundant... They reset the stack trace of the caught-rethrown expections, therefore hiding where the exception really originated.

     

    Surely that's worse than redundant?

     



  • @Sunday Ironfoot said:

    Surely that's worse than redundant?
    Sure. I wasn't defending the code. On the contrary.

    Oh, let it be me the first to point out how I wrote "expections" instead of "exceptions". I'm not giving anyone else that pleasure! Nya nya!



  • Here is the fix:

    public SqlDataReader DoSomething()
    {
    try
    {
    // Some code

        try
        {
            // Some more code
        }
        catch (Exception ex)
        {
            throw new InternalException(ex);
        }
        finally
        {
            // Some class that implements IDisposable
            dataReader.Close();
        }
    }
    catch (Exception ex)
    {
        throw new InternalException(ex);
    }
    

    }

    Now the stack trace is correctly preserved.



  •  An Expection is when the code is more likely to throw than to exit normally.



  • @henke37 said:

    Here is the fix: public SqlDataReader DoSomething() { try { // Some code try { // Some more code } catch (Exception ex) { throw new InternalException(ex); } finally { // Some class that implements IDisposable dataReader.Close(); } } catch (Exception ex) { throw new InternalException(ex); } } Now the stack trace is correctly preserved.
    Despite your formatting, I hope that's a joke.



  • Of course it is a joke, it fixes the wrong problem.



  • @zelmak said:

    each open/close pairing should have its closing in its own finally block

    Indeed, but are all those catch clauses really necessary?

    Connection conn = DriverManager.getConnection(...);
    try {
      PreparedStatement ps = conn.prepareStatement(...);
      try {
        ResultSet rs = ps.executeQuery();
        try {
          while(rs.next()) {
            // work with the ResultSet
          }
        } finally {
          rs.close();
        }
      } finally {
        ps.close();
      }
    } finally {
      conn.close();
    }
    @zelmak said:
    TRWTF is having to handle exceptions on .close() actions

    +1



  • @henke37 said:

    Of course it is a joke, it fixes the wrong problem.
    ... If you were really trying to fix the problem of having the stack trace, you would have simply written "throw;"



  • @Sunday Ironfoot said:

    using(SqlDataReader reader = //Code to get DataReader)

    {

        // Some code here

        return reader;

    }

     

    With this code, won't reader be Dispose'd before the caller gets the return value? This seems analogous to, in C++, returning a reference to a destructed object (i.e. a bad idea)



  • @fatbull said:

    @zelmak said:

    each open/close pairing should have its closing in its own finally block

    Indeed, but are all those catch clauses really necessary?

    Only if you care about logging errors so you can find out why your app is borken while running live ... otherwise, 🤷, guess not.



  • @smxlong said:

    @Sunday Ironfoot said:

    using(SqlDataReader reader = //Code to get DataReader)

    {

        // Some code here

        return reader;

    }

     

    With this code, won't reader be Dispose'd before the caller gets the return value? This seems analogous to, in C++, returning a reference to a destructed object (i.e. a bad idea)
    Yes


  • I have this in one production system (written in Python):

    try:
      # Some stuff that might raise an exception
    except:
      try:
        # Attempt to dump exception information in a file
      except:
        pass  # Couldn't write the file for whatever reason, oh well
      raise  # Let other code see the original exception
    
    @zelmak said:
    TRWTF is having to handle exceptions on .close() actions ... I mean ... seriously ... if the thing won't close, what else is there to do?

    TRWTF is a language that doesn't support RAII (well, I guess IDisposable kinda does that - but that apparently requires a special syntax).



  •  Now you can see why people hate JDBC and how ORMs got such an early start in Java.



  • @smxlong said:

    @Sunday Ironfoot said:

    using(SqlDataReader reader = //Code to get DataReader)

    {

        // Some code here

        return reader;

    }

     

    With this code, won't reader be Dispose'd before the caller gets the return value? This seems analogous to, in C++, returning a reference to a destructed object (i.e. a bad idea)

     

    You're right, it's a badly anonymized code sample, real code doesn't actually do that.

     



  • Just a quick update to this.

    I managed to trim 1500 lines of code from a class by deleting redundant try/catch statements and converting try/catch/finally into 'using'

    But I suppose the real WTF was that the class had over 5000 lines of code to begin with.

    Seriously, this whole application is a gold mine of WTF's.



  • @Sunday Ironfoot said:

    Just a quick update to this.

    I managed to trim 1500 lines of code from a class by deleting redundant try/catch statements and converting try/catch/finally into 'using'

    But I suppose the real WTF was that the class had over 5000 lines of code to begin with.

    Seriously, this whole application is a gold mine of WTF's.

    Really ... 5,000? ONLY 5,000?

    I recently refactored a file with 55K (yes ... 55 THOUSAND) lines of crap DOWN to 5,000-ish. Yes, there was a lot of commented-out code, but most of it wasn't ...

    Sadly, this class further extended a class that was already ~30K lines itself (hence the reason for the extension instead of shoving even more code into the original class.)

    My co-workers now adore me for making (most) of the database-side coding more simple ... NetBeans would normally balk at opening such a file and would lockup when it had to reparse it internally to display the 'navigator' view. I wish NetBeans had a 'move Method' refactoring tool ... I had to do it all by hand ... :(



  • @smxlong said:

    @Sunday Ironfoot said:

    using(SqlDataReader reader = //Code to get DataReader)

    {

        // Some code here

        return reader;

    }

     

    With this code, won't reader be Dispose'd before the caller gets the return value? This seems analogous to, in C++, returning a reference to a destructed object (i.e. a bad idea)

    Yes, you'd probably end up with an ObjectDisposedException. In reality you'd probably write something like the following:

    public void DoSomething()
    {
      using (var dataReader = GetSqlDataReader())
      {
        dataReader.DoSomething();
    
    DoSomethingElse(dataReader);
    

    }
    }



  • @Sunday Ironfoot said:

    @boomzilla said:

    ....but if you're saying this is a WTF because he could have used the syntactic sugar of using, then this is more of a mug at best, and pedantic dickweedery to be sure.

     No! The WTF is that the try/catch blocks are completely redundant because he's just rethrowing the exceptions up the call stack. I just got sidetracked by the using stuff.

    NOT completely redundant since "throw ex" updates the reported stack frame to the current location, and eliminates exposure of the original location. If you want to maintain things then just "throw;"

    Also, if there is a possibility the the close in the inner finally will throw,,then the above might be useful [very doubtful, but not impossible]



  • @TheCPUWizard said:

    @Sunday Ironfoot said:

    @boomzilla said:

    ....but if you're saying this is a WTF because he could have used the syntactic sugar of using, then this is more of a mug at best, and pedantic dickweedery to be sure.

     No! The WTF is that the try/catch blocks are completely redundant because he's just rethrowing the exceptions up the call stack. I just got sidetracked by the using stuff.

    NOT completely redundant since "throw ex" updates the reported stack frame to the current location, and eliminates exposure of the original location. If you want to maintain things then just "throw;"

    Also, if there is a possibility the the close in the inner finally will throw,,then the above might be useful [very doubtful, but not impossible]

     

    This has already been covered by someone else. Basically, resetting the stack trace is argubly worse than just having a throw, because you lose information about the origin of the exception.

    Also, I'm sure the .NET framework engineers are competant enough to ensure the .Dipose() or .Close() methods won't throw an exception. Indeed it's normal practice to wrap any type that implements IDisposal in a using statement, that's what it's there for.

     



  • @zelmak said:

    @Sunday Ironfoot said:

    Just a quick update to this.

    I managed to trim 1500 lines of code from a class by deleting redundant try/catch statements and converting try/catch/finally into 'using'

    But I suppose the real WTF was that the class had over 5000 lines of code to begin with.

    Seriously, this whole application is a gold mine of WTF's.

    Really ... 5,000? ONLY 5,000?

    I recently refactored a file with 55K (yes ... 55 THOUSAND) lines of crap DOWN to 5,000-ish. Yes, there was a lot of commented-out code, but most of it wasn't ...

    Sadly, this class further extended a class that was already ~30K lines itself (hence the reason for the extension instead of shoving even more code into the original class.)

    My co-workers now adore me for making (most) of the database-side coding more simple ... NetBeans would normally balk at opening such a file and would lockup when it had to reparse it internally to display the 'navigator' view. I wish NetBeans had a 'move Method' refactoring tool ... I had to do it all by hand ... :(

     

    Was the 55k loc to do with database access? This area is where I see the worst violations of the DRY principle (Don't Repeat Yourself). Normally you'd use an ORM to overcome this, but my company isn't keen on them because they think ORM's are too slow

     



  • @Sunday Ironfoot said:

    Was the 55k loc to do with database access? This area is where I see the worst violations of the DRY principle (Don't Repeat Yourself). Normally you'd use an ORM to overcome this, but my company isn't keen on them because they think ORM's are too slow

    Yep. And a significant amount of the code predates anything considered an ORM. Everything else was glommed on in the same manner since it was the pattern that existed.



  • Perhaps the code used to look like this:

        public SqlDataReader DoSomething()
        {
            try
            {
                // Some code
       
                try
                {
                    // Some more code
                }
                catch (Exception ex)
                {
                    log.error("something bad happened!", ex);
                    throw ex;
                }
                finally
                {
                    // Some class that implements IDisposable
                    dataReader.Close();
                }
            }
            catch (Exception ex)
            {
                log.error("something bad happened!", ex);
                throw ex;
            }
        }

     Then someone removed the superfluous error logging but didn't yank the unnecessary try-catch blocks?



  • @tdb said:

    TRWTF is a language that doesn't support RAII (well, I guess IDisposable kinda does that - but that apparently requires a special syntax).

    QFTT. The problem is that so few people realize the difference between RAII and IDisposable. It's like requiring external locking or not. Not to talk about nesting disposable resources. Here RAII languages are a HUGE win. I would give up my left arm if I could get more people (mostly language designers) to grok this. And yes, I am left handed.

    Also, @arotenbe said:

    try-finally < using block < lambda callbacks < custom monads <= RAII
    FTFY



  • The more I hear people preach RAII, the less inclined I am to use it.



  • @blakeyrat said:

    The more I hear people preach RAII, the less inclined I am to use it.

    Who is preaching it? We must be from different planets. Most people I talk to basically just go "BAH". Also, most people I talk to only ever think about starting their program, not shutting it down or keeping it running. So I've concluded that that is the crowd I must fight, which would be a losing battle then.



  • Some empirical evidence:

    - about 5m results

    - about 2m results



  • @blakeyrat said:

    The more I hear people preach RAII, the less inclined I am to use it.

    Ouch. Brain-hurt. Okay, so I've never heard the acronym RAII until this thread, and I just got through [most of] the wiki article about it. From what I gather, it's a sort of idiom that promotes behind-the-scene resource management - the examples used were opening, writing to, and closing files. I can also see this with database access as well. So, with that basic understanding of RAII, I'm all for it. But, I can also see how it could be misused/abused and cause nothing but pain and suffering.



  • @Obfuscator said:

    Also, @arotenbe said:
    try-finally < using block < lambda callbacks < custom monads <= RAII
    FTFY
    I would never put RAII above lambda callbacks. The former are much more broadly useful than the latter.

    In fact, you can use lambda callbacks to implement RAII (as I see it).

     

    function UseResource(resourceDescription, callback){
        try {
            Resource res = null;
            res = GetResource(resourceDescription);
            callback(res);
        }
        finally {
            if (res)
                close(res);
        }
    }

     

    Heck, even the Wikipedia article on RAII mentions this.



  • I've actually read about it, and I think it's generally a good idea-- I just have a natural knee-jerk reaction against hype. You get a lot of RAII hype in programming boards, which always makes me think: "hm, if this were really so good, why would people feel the need to hype it?"



  • RAII is quite simple and powerful, but almost always over complicated. If "Resource Allocation IS Initialization" then it means you NEVER have an "uninitialized" object. Period. Simple. If an insance can not be properly initialized to "do what it is supposed to" then the "allocation" itself fails, and you never get to see the instance.



  • Not true. This deals with the useage of a single resource, but not a whole tree structure like RAII in c++ does. Lambda callbacks are not any better than the using-statement of C#. Not saying that it is bad, just that RAII is a more complete solution.



  • @Zecc said:

    @Obfuscator said:

    Also,
    @arotenbe said:
    try-finally < using block < lambda callbacks < custom monads <= RAII

    FTFY
    I would never put RAII above lambda callbacks. The former are much more broadly useful than the latter.

    In fact, you can use lambda callbacks to implement RAII (as I see it).

     

    function UseResource(resourceDescription, callback){
    try {
    Resource res = null;
    res = GetResource(resourceDescription);
    callback(res);
    }
    finally {
    if (res)
    close(res);
    }
    }

     

    Heck, even the Wikipedia article on RAII mentions this.

    While this certainly is a working RAII implementation, it strikes me as rather cumbersome if you want to use multiple resources at once, or if you need to use them in a longer piece of code. I imagine it would look something like this (using OpenGL rendering as an example, and RAII at the binding level):

    function render_something()
    {
      UseResource(texture, function() {
        UseResource(shader, function() {
          UseResource(vertex_buffer, function() {
            /* Draw objects using the bound resources */
          })
        })
      })
    }
    

    Compare with the C++ approach:

    struct UseResource
    {
      Resource &res;
      UseResource(Resource &r): res(r) { res.bind(); }
      ~UseResource() { res.unbind(); }
    };
    

    void render_something()
    {
    UseResource use_tex(texture);
    UseResource use_shader(shader);
    UseResource use_vbuf(vertex_buffer);

    /* Now do the rendering */
    }

    The drawback is that you need to give names to the RAII objects; unfortunately the language doesn't support anonymous scoped objects.

    Another example could be database access. RAII fits several concepts, including connecting to the database, locking tables and wrapping stuff in transactions. Often you'll need to nest these operations, and in complex applications there can be quite a bit of code in a single transaction. I'll let you imagine what the code would look like in each case.

    Notice also the lack of explicit exception handling in the C++ example. The major feature in C++ that allows very simple RAII implementations is that the language is very specific about the lifetime of objects. With stack objects (they are said to have "automatic storage duration") you can tell exactly when they go out of scope and they get destroyed. With garbage-collecting languages, it's generally "sometime in the not-too-distant future, if I feel like it", which makes the destructors/finalizers/whatever useless for RAII purposes. It won't do to have a lingering RAII object get destroyed and unbind your texture when you've already bound a new one and are rendering with it.



  • Alright, I admit I didn't think of when you have to deal with multiple resources.


Log in to reply