So You Want to Practice your Code Reviewing Skills? - Summary

Earlier this week I published So You Want to Practice your Code Reviewing Skills? which challenged the readers to find bugs in ~ 150 LOC.


The replies were very interesting. Here's a summary

Concurrency/Distribution/Singleton

• instance() method is not synchronized
• sessionCounter needs to be volatile/guarded by synchronization (in visitorCount())
• (Mutable) Singeltons cannot be distributed. is If there is several instance of the webapp running, not all values will be counted because several counter will exist and not be synchronized from JVM to JVM.
• The singleton implementation is broken. See, for instance, this article.

General

• DataBaseHelperget() method may return the empty string ("") if no rows are in the table, and the singleton only expects a valid number or null, which will cause a NumberFormatException
• the counter may roll over. For a 7 year old web app this is perfectly possible.
• I consider constructs like this catch(Exception e) { sLogger.error("", e); } to be a bug.
  

JDBC

• The DB change isn't being committed so it has to rely on the underlying driver's behavior.
• The insert statement doesn't explicitly defines column names, which is susceptible to failure if the order of rows in the DB table changes.
• The update statement updates the row with param = 'SESSION_COUNT' although the insert inserts 'session_count'. Usually this would not work without special set-up in the db and/or connection.
• generateResult() does not close the ResultSet object. Note that the originating Statement object is hidden inside the DataBase.performSqlQuery() method (whose code is not given) so Statement also remains open. The closing of these two objects is deferred to the GC.
  

Final thoughts

• Do not use singletons. A singleton is a smell that indicates the need for dependency injection.
  
• In general, service objects in web-apps should not maintain state in (plain-old-Java) fields. If you need to have some state, write it to the DB. That's the only way to share state in a distributed settings.
• The nastiest bug of all: the combination of exceptions being silently absorbed, and DataBaseHelper.get() returning an empty string ("") leads to a situation where sessionCounter stays zero, which will reset the visitor count at the DB to 100, thereby loosing the (correct) visitor count.
  
  Here's the scenario: In generateResult() an SqlException if fired (let's say due to a temporary network problem). It is silently caught by generateResult() which then returns an empty list. get() will return an empty string, which will yield a NumberFormatException in reloadParamsFromDB(), which - again - is silently ignored.
  Therefore, no assignment to sessionCounter is taking place, so it retains its default value, zero. At the 100th call to addSession() sessionCounter current value (100) will be written to the DB.
  
  Solution: add to the value at the DB instead of writing to it.

So You Want to Practice your Code Reviewing Skills?

The code below is taken from a real web-app that has been up and running for > 7 years. This specific fragment is realizing the visitor count functionality: keeping track on the number of visitors hitting the site. Each time a new session is created SiteInfo.instance().addSession() is called.

Your task (should you choose to accept it...) is to find bugs in this code. In other words: will the visitor count, (as reported by SiteInfo.visitorCount()) always be correct? If not, what values can be seen there? How can we fix the code?

Please ignore design issues (I don't like the singleton anymore than you do), or technological issues, such as: "well you should just rewrite the whole thing with Spring + Hibernate". Just examine the code-as is and try to determine if/where can it fail.

public class SiteInfo {
   
   private static SiteInfo inst;
      
   private SiteInfo() {
      //
   };
   
   // This is a singleton (Yak!)
   public static SiteInfo instance() {
      if(inst == null) 
         inst = new SiteInfo();
      return inst;
   }

   private int sessionCounter = 0; 
   private static final String INIT = "100";
      
   public int visitorCount() {
      return sessionCounter;
   }
      
   public synchronized void reloadParamsFromDB() {
      Connection con = null;
      ConnectionPool pool = null;
      try {

         pool = ConnectionPoolFactory.getInstance();
         con = pool.getConnection();

         String countStr = DataBaseHelper.get(con, "config", "value",
            "param", "session_count"); 
         if (countStr == null) {
            String q = "INSERT INTO config VALUES ('session_count','" + INIT + "')";
            try {
               runCommand(q, con);
               countStr = INIT;
            }
            catch (Exception e) {
               log.error("", e);
            }
         }

         sessionCounter = Integer.parseInt(countStr);
      }
      catch (Exception e) {
         log.error("", e);
      }
      finally {
         ConnectionPoolFactory.release(pool, con);
      }
   }
   
   private void runCommand(String q, Connection con) throws Exception {
      Statement stmt = con.createStatement();
      try {
         stmt.executeUpdate(q);         
      }
      finally {
         stmt.close();
     }
   }
   
   public synchronized void addSession() {
      sessionCounter++;
      if(sessionCounter % 100 != 0)
         return;

      Connection con = ConnectionPoolFactory.getInstance().getConnection();
      Statement stmt = null;
      try {
         stmt = con.createStatement();
         String sql = "UPDATE config SET value='" + sessionCounter 
            + "' WHERE param='SESSION_COUNT'";
         stmt.execute(sql);
      }
      catch (Exception ex) {
         log.error("", ex);
      }
      finally {
         try {
            if(stmt != null)
               stmt.close();
            ConnectionPoolFactory.release(con);
         }
         catch(SQLException e) {
            log.error("", e);
         }
      }
   }   
}

public class DataBaseHelper {
   public static String get(Connection con, String table, 
      String columnA, String columnB, String columnAValue) {
      String result = "";
      String sqlQuery = "SELECT " + columnB + " from " + table
            + " WHERE " + columnA + " = '" + columnAValue + "'";

      // Run the query. Translate the result set into a list of maps.
      // Each map corresponds to a single row in the ResultSet
      List<Map<Object,Object>> rows = generateResult(sqlQuery, con);
      try {
         Iterator<Map<Object,Object>> iter = rows.iterator();
         if(iter.hasNext()) {
           Map<Object,Object> m = iter.next();
         
           result = (String) (m.get(columnB));
           if (result == null)
              return null;
         }
      } 
      catch (Exception e) {
         return null;
      }
      return result;
   }  

   public static List<Map<Object,Object>> generateResult(String query, Connection con) {
      List<Map<Object,Object>> result = new ArrayList<Map<Object,Object>>();
      try {
         ResultSet resultSet = DataBase.performSqlQuery(query, con);
         if(resultSet == null)
            throw new Exception("Impossible"); 
            
         ResultSetMetaData resultSetMetaData = resultSet.getMetaData();         
         int columnCount = resultSetMetaData.getColumnCount();
         
         String[] columnNames = new String[columnCount];
         for(int i = 0; i < columnCount; i++)
            columnNames[i] = resultSetMetaData.getColumnName(i + 1);
            
         while(resultSet.next()) {
            Map<Object,Object> map = new HashMap<Object,Object>();
            for(int i = 0; i < columnCount; i++) {
               String col = columnNames[i];
               map.put(col, resultSet.getString(i + 1));
            }

            result.add(map);
         }
      }
      catch(Exception e) {
         sLogger.error("", e);
      }

      return result;
   }
}