Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.java.programmer > #36606 > unrolled thread

Eclipse doesn't like else

Back to article view | Back to comp.lang.java.programmer

Contents

  Eclipse doesn't like else Eric Douglas <e.d.programmer@gmail.com> - 2017-05-12 10:44 -0700
    Re: Eclipse doesn't like else Marcel Mueller <news.5.maazl@spamgourmet.org> - 2017-05-12 20:30 +0200
      Re: Eclipse doesn't like else Eric Douglas <e.d.programmer@gmail.com> - 2017-05-12 11:53 -0700
        Re: Eclipse doesn't like else Arne Vajhøj <arne@vajhoej.dk> - 2017-05-13 20:39 -0400
          Re: Eclipse doesn't like else v_borchert@despammed.com (Volker Borchert) - 2017-05-14 03:50 +0000
    Re: Eclipse doesn't like else v_borchert@despammed.com (Volker Borchert) - 2017-05-12 18:15 +0000
      Re: Eclipse doesn't like else Eric Douglas <e.d.programmer@gmail.com> - 2017-05-12 13:33 -0700
    Re: Eclipse doesn't like else Martin Gregorie <martin@address-in-sig.invalid> - 2017-05-12 21:22 +0000
    Re: Eclipse doesn't like else v_borchert@despammed.com (Volker Borchert) - 2017-05-13 00:52 +0000
    Re: Eclipse doesn't like else Arne Vajhøj <arne@vajhoej.dk> - 2017-05-13 21:04 -0400

#36606 — Eclipse doesn't like else

Why must it give a warning on this else condition?

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.SQLException;

public class CompileMe {
     public static void testMe() {
          try {
               Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password");
               String someCondition = "check me";
               PreparedStatement myPrep;
               myPrep = conn.prepareStatement("initial query");
               myPrep.executeQuery();
               myPrep.close();
               if (someCondition.equals("one")) {
                    myPrep = conn.prepareStatement("query something");
               } else {
                    myPrep = conn.prepareStatement("query something else");
               }
               myPrep.executeQuery();
               myPrep.close();
          } catch (SQLException e) {
               e.printStackTrace();
          }
     }
}

This example will run the same whether we have 2 calls to prepareStatement, or set one common string to pass into prepareStatement after the if, but if we write it this way Eclipse gives a warning "Resource leak: 'myPrep' is not closed at this location".

Take out the 3 lines above the if (prepare, execute, and close a different query) and the warning goes away.

[toc] | [next] | [standalone]

#36607

On 12.05.17 19.44, Eric Douglas wrote:
> Why must it give a warning on this else condition?
>
> public class CompileMe {
>       public static void testMe() {
>            try {
>                 Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password");
>                 String someCondition = "check me";
>                 PreparedStatement myPrep;
>                 myPrep = conn.prepareStatement("initial query");
>                 myPrep.executeQuery();
>                 myPrep.close();
>                 if (someCondition.equals("one")) {
>                      myPrep = conn.prepareStatement("query something");
>                 } else {
>                      myPrep = conn.prepareStatement("query something else");
>                 }
>                 myPrep.executeQuery();
>                 myPrep.close();
>            } catch (SQLException e) {
>                 e.printStackTrace();
>            }
>       }
> }
>
> This example will run the same whether we have 2 calls to prepareStatement, or set one common string to pass into prepareStatement after the if, but if we write it this way Eclipse gives a warning "Resource leak: 'myPrep' is not closed at this location".

Eclispe is right. Your code is not exception safe. If an SQLException is 
thrown close will not be executed. close should always be called from a 
finally block.

Or even better with Java 7:

try (PreparedStatement myPrep = conn.prepareStatement("initial query"))
{ myPrep.executeQuery();
}


Marcel

[toc] | [prev] | [next] | [standalone]

#36608

On Friday, May 12, 2017 at 2:30:37 PM UTC-4, Marcel Mueller wrote:
> 
> Eclispe is right. Your code is not exception safe. If an SQLException is 
> thrown close will not be executed. close should always be called from a 
> finally block.
> Marcel

Which seems odd that it doesn't complain if you take out the 3 lines above the if, or if you pass the query as a variable so you can execute those 2 prepareStatement commands as one after the if, either of which would still produce the resource leak if it hits the catch clause.  It does like that method if it's coded like this.

          PreparedStatement myPrep = null; 
          try { 
               Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password"); 
               String someCondition = "check me"; 
               myPrep = conn.prepareStatement("initial query"); 
               myPrep.executeQuery(); 
               myPrep.close(); 
               if (someCondition.equals("one")) { 
                    myPrep = conn.prepareStatement("query something"); 
               } else { 
                    myPrep = conn.prepareStatement("query something else"); 
               } 
               myPrep.executeQuery(); 
               myPrep.close(); 
          } catch (SQLException e) { 
               e.printStackTrace(); 
          } finally {
               try {
                    myPrep.close();
               } catch (SQLException e) {
                    e.printStackTrace();
               } 
          }

[toc] | [prev] | [next] | [standalone]

#36613

On 5/12/2017 2:53 PM, Eric Douglas wrote:
> On Friday, May 12, 2017 at 2:30:37 PM UTC-4, Marcel Mueller wrote:
>> Eclispe is right. Your code is not exception safe. If an
>> SQLException is thrown close will not be executed. close should
>> always be called from a finally block. Marcel
>
> Which seems odd that it doesn't complain if you take out the 3 lines
> above the if, or if you pass the query as a variable so you can
> execute those 2 prepareStatement commands as one after the if, either
> of which would still produce the resource leak if it hits the catch
> clause.

I don't think Eclipse detects all cases of resource leak.

It is still your responsibility.

Arne

[toc] | [prev] | [next] | [standalone]

#36616

Arne Vajh??j wrote:

> I don't think Eclipse detects all cases of resource leak.

Obviously not, because it should complain abount the Connection as well.

-- 

"I'm a doctor, not a mechanic." Dr Leonard McCoy <mccoy@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert  <v_borchert@despammed.com>

[toc] | [prev] | [next] | [standalone]

#36609

Eric Douglas wrote:
> Why must it give a warning on this else condition?
> 
> import java.sql.Connection;
> import java.sql.DriverManager;
> import java.sql.PreparedStatement;
> import java.sql.SQLException;
> 
> public class CompileMe {
>      public static void testMe() {
>           try {
>                Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password");
>                String someCondition = "check me";
>                PreparedStatement myPrep;
>                myPrep = conn.prepareStatement("initial query");
>                myPrep.executeQuery();
>                myPrep.close();
>                if (someCondition.equals("one")) {
>                     myPrep = conn.prepareStatement("query something");
>                } else {
>                     myPrep = conn.prepareStatement("query something else");
>                }
>                myPrep.executeQuery();
>                myPrep.close();
>           } catch (SQLException e) {
>                e.printStackTrace();
>           }
>      }
> }
> 
> This example will run the same whether we have 2 calls to prepareStatement, or set one common string to pass into prepareStatement after the if, but if we write it this way Eclipse gives a warning "Resource leak: 'myPrep' is not closed at this location".
> 
> Take out the 3 lines above the if (prepare, execute, and close a different query) and the warning goes away.

What about
    myPrep = conn.prepareStatement(someCondition.equals("one")
                                   ? "query something"
                                   : "query something else");


PS: Lines of postings should ideally not exceed 72 characters,
    to leave room for a couple of quoting levels before reaching
    the traditional 80 columns of terminals, xterm, and the like.

-- 

"I'm a doctor, not a mechanic." Dr Leonard McCoy <mccoy@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert  <v_borchert@despammed.com>

[toc] | [prev] | [next] | [standalone]

#36610

On Friday, May 12, 2017 at 3:05:57 PM UTC-4, Volker Borchert wrote:
> What about
>     myPrep = conn.prepareStatement(someCondition.equals("one")
>                                    ? "query something"
>                                    : "query something else");
> 
> 

Yes, that is also acceptable.  That is a very similar result to the
String qry;
if (someCondition.equals("one")) {
qry = "something";
} else {
qry = "something else";
}
myPrep = conn.prepareStatement(qry);

except that I also translate some of this code into another hybrid language which integrates with Java and accepts calls to Java methods but has it's own syntax for conditions which doesn't accept the ? condition.  Otherwise it's possible to consider this the cleanest version.

[toc] | [prev] | [next] | [standalone]

#36611

On Fri, 12 May 2017 10:44:34 -0700, Eric Douglas wrote:

> Why must it give a warning on this else condition?
> 
> import java.sql.Connection;
> import java.sql.DriverManager;
> import java.sql.PreparedStatement;
> import java.sql.SQLException;
> 
> public class CompileMe {
>      public static void testMe() {
>           try {
>                Connection conn =
>                DriverManager.getConnection("db_connect_string",
>                "db_userid", "db_password");
>                String someCondition = "check me";
>                PreparedStatement myPrep;
>                myPrep = conn.prepareStatement("initial query");
>                myPrep.executeQuery();
>                myPrep.close();
>                if (someCondition.equals("one")) {
>                     myPrep = conn.prepareStatement("query something");
>                } else {
>                     myPrep = conn.prepareStatement("query something
>                     else");
>                }
>                myPrep.executeQuery();
>                myPrep.close();
>           } catch (SQLException e) {
>                e.printStackTrace();
>           }
>      }
> }
> 
> This example will run the same whether we have 2 calls to
> prepareStatement, or set one common string to pass into prepareStatement
> after the if, but if we write it this way Eclipse gives a warning
> "Resource leak: 'myPrep' is not closed at this location".
> 
> Take out the 3 lines above the if (prepare, execute, and close a
> different query) and the warning goes away.

I'd probably pull the repeated JDBC statements out into a separate 
method, something like:

private void doQuery(String query) throws SQLException
{
  try
  {
    PreparedStatement myPrep;
    myPrep = conn.prepareStatement(query);
    myPrep.executeQuery();
    myPrep.close();
  }
  catch (SQLException e)
  {
    /*
       Deal with anything handleable at point before re-throwing 
       the query
    */
    throw e;
  }
}

and then call it three times, though doubtless in RealLife(TM) it would 
need to be a bit more complex because your example code doesn't show what 
is done with the query's results. Similarly, the method's catch block may 
be unnecessary if it cant add value by by being there.


-- 
martin@   | Martin Gregorie
gregorie. | Essex, UK
org       |

[toc] | [prev] | [next] | [standalone]

#36612

Eric Douglas wrote:
> Why must it give a warning on this else condition?
> 
> import java.sql.Connection;
> import java.sql.DriverManager;
> import java.sql.PreparedStatement;
> import java.sql.SQLException;
> 
> public class CompileMe {
>      public static void testMe() {
>           try {
>                Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password");
>                String someCondition = "check me";
>                PreparedStatement myPrep;
>                myPrep = conn.prepareStatement("initial query");
>                myPrep.executeQuery();
>                myPrep.close();
>                if (someCondition.equals("one")) {
>                     myPrep = conn.prepareStatement("query something");
>                } else {
>                     myPrep = conn.prepareStatement("query something else");
>                }
>                myPrep.executeQuery();
>                myPrep.close();
>           } catch (SQLException e) {
>                e.printStackTrace();
>           }
>      }
> }
> 
> This example will run the same whether we have 2 calls to prepareStatement, or set one common string to pass into prepareStatement after the if, but if we write it this way Eclipse gives a warning "Resource leak: 'myPrep' is not closed at this location".
> 
> Take out the 3 lines above the if (prepare, execute, and close a different query) and the warning goes away.

Now let's combine the suggestions (note: did not try to actually compile):

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

public class CompileMe {
     private static void query(final Connection conn, final String sql) throws SQLException {
          try (final PreparedStatement stmt = conn.prepareStatement(sql)) {
               try (final ResultSet rslt = stmt.executeQuery()) {
                    // do something with result.
                    // try-with-resources will take care of
                    // close()ing the ResultSet when done.
               }
          }
     }
 
     public static void testMe() {
          try (final Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password")) {
               String someCondition = "check me";
               query(conn, "initial query");
               if (someCondition.equals("one")) {
                    query(conn, "query something");
               } else {
                    query(conn, "query something else");
               }
          } catch (SQLException e) {
               e.printStackTrace();
          }
     }
}


-- 

"I'm a doctor, not a mechanic." Dr Leonard McCoy <mccoy@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert  <v_borchert@despammed.com>

[toc] | [prev] | [next] | [standalone]

#36614

On 5/12/2017 1:44 PM, Eric Douglas wrote:
> Why must it give a warning on this else condition?
>
> import java.sql.Connection;
> import java.sql.DriverManager;
> import java.sql.PreparedStatement;
> import java.sql.SQLException;
>
> public class CompileMe {
>      public static void testMe() {
>           try {
>                Connection conn = DriverManager.getConnection("db_connect_string", "db_userid", "db_password");
>                String someCondition = "check me";
>                PreparedStatement myPrep;
>                myPrep = conn.prepareStatement("initial query");
>                myPrep.executeQuery();
>                myPrep.close();
>                if (someCondition.equals("one")) {
>                     myPrep = conn.prepareStatement("query something");
>                } else {
>                     myPrep = conn.prepareStatement("query something else");
>                }
>                myPrep.executeQuery();
>                myPrep.close();
>           } catch (SQLException e) {
>                e.printStackTrace();
>           }
>      }
> }
>
> This example will run the same whether we have 2 calls to
> prepareStatement, or set one common string to pass into prepareStatement
> after the if, but if we write it this way Eclipse gives a warning
> "Resource leak: 'myPrep' is not closed at this location".
>
> Take out the 3 lines above the if (prepare, execute, and close a
 > different query) and the warning goes away.

As already explained by other then there are a resource leak.

And as I said in another reply then I don't think Eclipse detects
all resource leaks.

But if we take a step back and look at the code then there are
several things that comes to my mind:
* I do not like the reuse of the PreparedStatement ref I would
   use different ref's
* all statements and connection need to be closed to avoid resource
   leak (well - statements should be closed when connection is
   closed per specs, but I would not rely on that)
* the only way to get this right is to have a systematic
   approach

Here are 3 different approaches:

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.SQLException;

public class ToCloseOrNotToclose {
     // Approach 1:
     // * close everything as soon as possible in good flow
     // * close in catch block if necessary to handle bad flows
     public static void approach1(String cond) {
         Connection con = null;
         PreparedStatement upd1 = null;
         PreparedStatement upd2a = null;
         PreparedStatement upd2b = null;
         try {
              con = DriverManager.getConnection("db_connect_string", 
"db_userid", "db_password");
              upd1 = con.prepareStatement("initial query");
              upd1.executeQuery();
              upd1.close();
              if (cond.equals("one")) {
                  upd2a = con.prepareStatement("query something");
                  upd2a.executeQuery();
                  upd2a.close();
              } else {
                  upd2b = con.prepareStatement("query something else");
                  upd2b.executeQuery();
                  upd2b.close();
              }
              con.close();
         } catch (SQLException e) {
             e.printStackTrace();
             try {
                 if(upd1 != null && !upd1.isClosed()) upd1.close();
                 if(upd2a != null && !upd2a.isClosed()) upd2a.close();
                 if(upd2b != null && !upd2b.isClosed()) upd2b.close();
                 if(con != null && !con.isClosed()) con.close();
             } catch (SQLException e2) {
                 e2.printStackTrace();
                 // consider what to do here
             }
             // consider what to do here
         }
     }
     // Approach 2:
     // * close in finally block for both good and bad flows
     public static void approach2(String cond) {
         Connection con = null;
         PreparedStatement upd1 = null;
         PreparedStatement upd2a = null;
         PreparedStatement upd2b = null;
         try {
              con = DriverManager.getConnection("db_connect_string", 
"db_userid", "db_password");
              upd1 = con.prepareStatement("initial query");
              upd1.executeQuery();
              if (cond.equals("one")) {
                  upd2a = con.prepareStatement("query something");
                  upd2a.executeQuery();
              } else {
                  upd2b = con.prepareStatement("query something else");
                  upd2b.executeQuery();
              }
              con.close();
         } catch (SQLException e) {
             e.printStackTrace();
             // consider what to do here
         } finally {
             try {
                 if(upd1 != null) upd1.close();
                 if(upd2a != null) upd2a.close();
                 if(upd2b != null) upd2b.close();
                 if(con != null) con.close();
             } catch (SQLException e2) {
                 e2.printStackTrace();
                 // consider what to do here
             }
         }
     }
     // Approach 3:
     // * use Java 7+ try with resource
     public static void approach3(String cond) {
         try (Connection con = 
DriverManager.getConnection("db_connect_string", "db_userid", 
"db_password")) {
              try(PreparedStatement upd1 = con.prepareStatement("initial 
query")) {
                  upd1.executeQuery();
              }
              if (cond.equals("one")) {
                  try(PreparedStatement upd2a = 
con.prepareStatement("query something")) {
                      upd2a.executeQuery();
                  }
              } else {
                  try(PreparedStatement upd2b = 
con.prepareStatement("query something else")) {
                      upd2b.executeQuery();
                  }
              }
         } catch (SQLException e) {
             e.printStackTrace();
             // consider what to do here
         }
     }
}

Arne

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.java.programmer

csiph-web