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.

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;
   }
}

Rules are a simple, yet amazingly powerful, mechanism introduced in JUnit version 4.7. They allow developers to easily customize JUnit's behavior by exposing meta information regarding the currently executing test. This post provides a straightforward example for writing a custom rule that augments JUnit with some useful functionality.

My subject class is IntSet: A set of integers implementing the standard operations of add(), remove(), contains(), clear() in O(1) time. To make this performance guarantee the set needs to know (in advance) the range of the values (min..max) and its size limit (number of elements that it will accommodate).

All in all, IntSet looks something like this:

public class IntSet {
  ... // Some private fields
  public IntSet(int limit, int min, int max) { ... }
  public int size() { ... }
  public boolean contains(int n) { ... }
  public void add(int n) { ... }
  public void remove(int n) { ... }
}

One of my unit tests specifies the behavior of IntSet when its size limit is reached. If I'm only interested in the type of the exception I can specify it via the expected attribute of the @Test annotation:

  @Test(expected=IllegalStateException.class)
  public void shouldNotExceedCapacity() {
    IntSet s = new IntSet(2, -10, 100);  // Set size limit to 2
    s.add(30);
    s.add(40);
    s.add(50);   // Insertion of the 3rd element should fail
  }

There are two drawbacks with this test. First, It only asserts the type of the an exception. It does not check the error message specified for the exception. Second, it does not assert that the exception was triggered by the last add() call. In other words, if we have a bug and the 2nd add() call is failing - with the same type of exception - the test will still pass.

To overcome this limitation we want to check the error message of the thrown exception. Specifically, we want to verify that the execution of the method fires an exception whose error message is "Cannot insert '50' - The set is full". Clearly, the chances of such an exception being thrown by the 2nd call are pretty slim.

Extending JUnit in such a manner is pretty easy thanks to the rules mechanism:

public class IntSet_Tests {

  @Retention(RetentionPolicy.RUNTIME)
  @Target(ElementType.METHOD)
  @interface Throwing {
    public String value();
  }
  
  @Rule
  public MethodRule mr = new MethodRule()
  {
    @Override
    public Statement apply(final Statement base, FrameworkMethod m, Object o) {
      Throwing t = m.getAnnotation(Throwing.class);
      if(t == null)
        return base;
      
      final String message = t.value();
      return new Statement() {
        
        @Override
        public void evaluate() throws Throwable {
          try {
            base.evaluate();
            fail("No exception was thrown");
          }
          catch(AssertionError e) {
            throw e;
          }
          catch(Exception e) {
            assertEquals("Incorrect exception message", message, e.getMessage());
          }
        }
      };
    }    
  };

  // All sort of @Test methods ...


  // And now, a method that asserts the error message
  @Throwing("Cannot insert '50' - The set is full")
  @Test
  public void shouldNotExceedCapacity() {
    IntSet s = new IntSet(2, -10, 100);
    s.add(30);
    s.add(40);
    s.add(50);
  }
}

First we define a new annotation, @Throwing. Then we define a field annotated with @Rule to provide the custom handling of this annotation. Finally, we annotate the shouldNotExceedCapacity() method with a @Throwing("Cannot insert '50' - The set is full") annotation.

The mechanism works as follows: before each test method is run, JUnit creates a Statement object which is merely a command object through which the acutal method can be invoked. JUnit passes this object along with a FrameworkMethod object (a wrapper of Java's Method) and the unit test instance to all @Rule fields defined at the test class.

A @Rule field must be public and must implement the MethodRule interface (of course, you can instead extend one of several classes conveniently defined by JUnit). In the apply() method, above, we create a new Statement object that wraps the original one. The new evaluate() method will check that if an exception is thrown its message matches the text specified by the @Throwing annotation attached to the method.

Obviously, there are other ways to do that. For instance, one can use the ExpectedException class (a predefined JUnit rule) to achieve a similar effect. The purpose of this post is to surface the (mighty) powers of JUnit meta programming.

I had already blogged about the Axioms of programming: the fundamental rules that govern the development of every piece of (substantial) software. In this post I want to focus to on the Instability Axiom:

The external behavior of a component will need to change over time

Here's a real story. I once was involved with a very a small project, let's call it the PQR project: three people working part time for three weeks, putting together an HTTP server a web client and a GUI client - all are very simple. During these three weeks we were also learning some new technologies so actual coding time (of all developers combined) was about 20-25 days.

During these three weeks two important changes were applied to PQR's specification:

• The technology with which the GUI client was implemented had to be changed. Instead of implementing it over Tech.A we had to switch to Tech.B.
  
  
• The initial specs defined the data that should be persisted by the server. As we were playing with the intermediate versions of the project, we came to realize that a descent user experience requires that additional information will be persisted.
  

The main point of this post is not if/how we managed to support these changes. The point is that even in small projects specs are not stable. We were not successful in defining the project's goals for a three week period in a project which is as simple as industrial projects get. Of course, If the project were more complicated (more people, wider scope) then the instability was likely to be even higher.

This example indicates that a "fire and forget" type of development (AKA: "divide and conquer" ) where one breaks down the desired functionality into a few large pieces, assigns each piece to a programmer, and then lets each programmer work on his task in isolation of his peers (until an "integration" milestone is approaching) is broken.

First, external forces will change the specs, thus affecting the assigned tasks. In the PQR project, the change in client technology was due to some external factors (business/marketing constraints). Even though the initial specs were examined and audited by several layers of approvals, no one had predicted this change.

Second, feedback from working early versions of the product (even with partial functionality) will change our understanding of the product and its desired capabilities. In PQR, the change regarding which-information-should-be-persisted was driven by experimentation with early versions.

If we had taken a Fire-and-Forget approach then our ability to respond to the first change were very limited as every team member was in the middle of his large task. Also, by the time a first working version were available, very little time was left to implement significant changes.

Bottom line: Software is unstable. Breaking the effort into tiny tasks with frequent integration points (I am speaking about granularity of hours not weeks) is an excellent way to cope with this inherent instability.