I am just wondering if this is a correct way to execute multiple query from a text file.

Connection conn = Utility.getConnection();
Statement stmt = null;
    JFileChooser chooser = new JFileChooser();
    int returnVal = chooser.showOpenDialog(chooser);
    if(returnVal == JFileChooser.APPROVE_OPTION) {
       String fileName=chooser.getCurrentDirectory().toString()+File.separator+chooser.getSelectedFile().getName();
       FileInputStream fstream = new FileInputStream(fileName);
       DataInputStream in = new DataInputStream(fstream);
       BufferedReader br = new BufferedReader(new InputStreamReader(in));
       String strLine;
       if(conn != null)
           stmt = conn.createStatement();
       while ((strLine = br.readLine()) != null)   {
           if(stmt !=null){
               int rc=stmt.executeUpdate(strLine);
               if(rc == 0)
                   System.out.println("executing:  "+strLine);
}catch (Exception e){
  • \$\begingroup\$ well you should try to execute it, and you will have a first answer. \$\endgroup\$
    – talnicolas
    Oct 24 '11 at 18:11
  • \$\begingroup\$ Code it running fine but I wanted to know some expert opinion :) \$\endgroup\$
    – Ashish Tyagi
    Oct 24 '11 at 18:18
  • 1
    \$\begingroup\$ It really depends on what the requirements are. Is it just to have an easy way to run a few test queries? Then it's probably fine (assuming it works). If it's for something more serious then I'm not too fond of your (lack of) error handling and I might be worried about security (even if not user supplied you might want to think twice about blindly running deletion or update queries) and atomicity. \$\endgroup\$
    – user786653
    Oct 24 '11 at 20:31
  • 1
    \$\begingroup\$ Side note: You might want to add extra try finally blocks around the execute code. There is no guarantee that the connection or stream is closed if an exception is thrown. \$\endgroup\$
    – Lionel
    Oct 25 '11 at 0:18

Some ideas:

  • Separate the GUI code from the logic.
  • If you stay with your current mixed code fail fast. If conn is null there no sense to open the file or ask the user to choose one.
  • JFileChooser.getSelectedFile() returns a File instance, pass it to FileInputStream directly, don't concatenate strings. File contains the absolute path.
  • It's a good practice to pass a Charset (or "UTF-8") the the constructor of the InputStreamReader. The default could vary from system to system.
  • The DataInputStream looks unnecessary. You can pass the FileInputStream to the InputStreamReader directly.
  • Consider using Guava's readLines.
  • Maybe you want to report all executed queries, so the if (rc == 0) condition is unnecessary.

Plus it lacks of proper exception/error handling and resource closing as others mentioned in the comments.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy