0
\$\begingroup\$

I am learning Java by reading source code of other authors. This Java method handles communication with SQL database. While the code does work several things concern me. For instance the try catch block at the end of the file used as a workaround. What is the correct way of doing it? What other problems can you spot?

    public void run() {
        if (running) {
            return;
        }
        running = true;            
        while(null == Common.server || null == Common.database || !ConnectionsPool.isInitialized()) {
            // Wait until the database is set before continuing...
            try {
                Thread.sleep(1000);
            }
            catch(Exception ex) {}
        }
        while(running) {
            final Connections cons = ConnectionsPool.getConnections();
            Connection  con  = null;
            while(!entries.isEmpty()) {
                if (null == con) {
                    con = cons.getConnection();
                }
                SQLLogEntry entry = entries.remove();
                if (null != entry) {
                    try {
                        write(entry, con); //find usages
                    }
                    catch (SQLException ex) {
                        writeLogFile("Could not write entry to SQL", ex);
                    }
                }
            }
            if (null != con) {
                try {
                    con.commit();
                }
                catch (SQLException ex) {
                    writeLogFile("Could not commit to SQL", ex);
                    try {
                        con.rollback();
                    }
                    catch (SQLException ex1) {
                    }

                    // log
                    final StringWriter errorOut = new StringWriter();                        
                    ex.printStackTrace(new PrintWriter(errorOut));
                    EditorTransactionUtil.writeLogFile(errorOut.toString());                        
                    // for user
                    final String msg = "Exception: " + EditorUtil.getErrorMessage(ex.getMessage());                        
                    try {
                        SwingUtilities.invokeAndWait(() -> {
                            JOptionPane.showMessageDialog(null, msg);
                        });
                    }
                    catch (Throwable ex1) {
                    }
                }
                finally {
                    cons.returnConnection(con);
                }
                con = null;
            }

            synchronized(entries) {
                try {
                    entries.wait(1000);
                }
                catch (InterruptedException ex) {
                    // This is a workaround to process this loop...
                }
            }
        }
        writeLogFile("SQLMsgLogger run stopping...");
    }
\$\endgroup\$
  • \$\begingroup\$ This is not your own code, is it? \$\endgroup\$ – Mast Aug 12 '16 at 12:50
  • \$\begingroup\$ This is an inherited code. \$\endgroup\$ – sixtytrees Aug 12 '16 at 12:58
  • \$\begingroup\$ This looks like an on-topic question because the OP is a maintainer of the code. \$\endgroup\$ – user34073 Aug 12 '16 at 17:33
  • \$\begingroup\$ Yes, I am maintaining it. I will fix concurrency check first. \$\endgroup\$ – sixtytrees Aug 12 '16 at 18:19
3
\$\begingroup\$

As a quick first note ( I will add more later when I have time), I assume the first lines;

   if (running) {
            return;
   }
   running = true;    

are attempting to ensure that this code will only be running once at a time... You would have to use a lock to ensure this as currently a race condition could cause;

1.) A thread checks running is false

2.) A second thread checks this before the first sets it to true

3.) Both threads continue

This is probably not what you want to happen....

Regarding your question about the InterruptedException, you should NOT just swallow it. I suggest reading the following article which covers the reasoning behind this: http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html?ca=drs-

You also seem to swallow Exceptions else where in the code (Once the base Exception class and not a subclass). This is a very bad habbit and you should consider which exceptions could happen and what that would imply for your code, then deal with them explicitly.

| improve this answer | |
\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.